Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libcurl 8.5.0 breaks mpd http streaming #12632

Closed
bubbleguuum opened this issue Jan 3, 2024 · 15 comments
Closed

libcurl 8.5.0 breaks mpd http streaming #12632

bubbleguuum opened this issue Jan 3, 2024 · 15 comments

Comments

@bubbleguuum
Copy link

bubbleguuum commented Jan 3, 2024

I did this

libcurl v8.5.0 is causing issues in mpd http streaming, for http streams whose size is > 128 MB (which causes mpd to not fully buffer the stream in memory). Shortly after playing such stream, a curl API inexplicably returns CURLE_OUT_OF_MEMORY, causing mpd to immediately stop playback. libcurl v8.4.0 and prior work just fine.

I reported all the details and findings about it and how to reproduce it on this mpd issue.

The problematic commit has been found to be 47f5b1a via bissecting.

After analysis, the mpd developer thinks this is a curl issue, hence this report.

Interestingly, mpd running with git libcurl (as of the date of this post) do not cause an OOM but instead will make the mpd "io" thread (making curl API calls) to take 100% CPU.

Any insights for fixing this issue would be appreciated.

I expected the following

No response

curl/libcurl version

libcurl 8.5.0

operating system

openSUSE Tumbleed (this issue is not OS dependent).

@bagder
Copy link
Member

bagder commented Jan 3, 2024

Pointing to a cryptic issue in another application is not helping us much. Please describe a lot more about the actual problem and how we can reproduce it - without running or debugging someone else's complicated application.

@bubbleguuum
Copy link
Author

mpd developer: not my problem
curl developer: not my problem

After spending several hours on this issue to narrow it down and writing reports, I'm sick of it. Feel free to close.
Let's keep mpd crashing.

@bagder
Copy link
Member

bagder commented Jan 3, 2024

A problem with curl is a problem for us, but for us to be able to work on a reported issue we need some guidance - a detailed description to start with. If neither you nor any other mpd user/developer want to help with that, then I cannot see how we can act on this report. Then it will be closed, yes. We are not mind-readers, unfortunately.

@bubbleguuum
Copy link
Author

If you want more details, ask mpd developer MaxKellerman, assuming he his willing to help, which I doubt.
I've done my part at this point.

@bagder
Copy link
Member

bagder commented Jan 3, 2024

Sorry, but if nobody can describe a curl problem to us then there is no curl problem as far as I am concerned.

@bubbleguuum
Copy link
Author

bubbleguuum commented Jan 3, 2024

Did you read this analysis from the mpd developer with the change in behavior between 6.4.0 and 6.5.0:

MusicPlayerDaemon/MPD#1952 (comment)

@bagder
Copy link
Member

bagder commented Jan 3, 2024

That's a description of mpd behavior when it uses libcurl together with some strace dumps. I have exactly zero knowledge of what mpd does with libcurl or how it works in general so I cannot tell what most of that means in terms of curl problems or not. What APIs does it use? What protocol (version) does it speak? What exactly is mpd doing when this issue is triggered? Which libcurl function returns what error?

@bagder bagder added the HTTP label Jan 3, 2024
@bubbleguuum
Copy link
Author

bubbleguuum commented Jan 3, 2024

All I know is that mpd uses the easy API for http streaming. I'll leave it to developers of 2 large software to eventually figure it out. I'm expert in neither the curl nor mpd codebase.

@jay
Copy link
Member

jay commented Jan 3, 2024

@MaxKellermann

@MaxKellermann
Copy link

MaxKellermann commented Jan 3, 2024

MPD uses the CURL "multi" API and uses CURLMOPT_SOCKETFUNCTION to integrate it in its epoll-based I/O event loop. See https://github.com/MusicPlayerDaemon/MPD/blob/master/src/lib/curl/Global.cxx for the part that does this integration. There is nothing special here, I guess every program using CURL does it that way, that's how CURL is supposed to be integrated in non-blocking I/O loops, is it not?

So the problem is that returning CURL_WRITEFUNC_PAUSE used to lead to a CURLMOPT_SOCKETFUNCTION call that would unregister the HTTP connection socket, effectively pausing the transfer (in CURL 8.4.0).
That CURLMOPT_SOCKETFUNCTION is no longer performed by CURL 8.5.0, and instead, CURL keeps receiving from the "paused" socket, buffering received data until the buffer eventually grows too large, causing dyn_nappend() to return CURLE_OUT_OF_MEMORY, failing the whole HTTP request.

I expect CURL_WRITEFUNC_PAUSE to effectively pause the transfer and never grow the dynbuf too large.

CURL 8.5.0 does not actually pause the transfer and not receive any data; but all it actually does since 8.5.0 is omit the WRITEFUNCTION calls. But it really keeps receiving+buffering data.

This is a CURL regression; at best, this is a semantic ABI change which breaks at least one widely used application after the update. But I believe this is really a bug, not just some minor semantic change.

@bagder
Copy link
Member

bagder commented Jan 3, 2024

@MaxKellermann HTTP/1 or HTTP/2 ?

@MaxKellermann
Copy link

I only tested with 1.

icing added a commit to icing/curl that referenced this issue Jan 4, 2024
- do not add a socket for POLLIN when the transfer does not
  want to send (for example is paused).
- refs curl#12632
@icing
Copy link
Contributor

icing commented Jan 4, 2024

@MaxKellermann it would be lovely if you could check #12633 out for a test.

@MaxKellermann
Copy link

@icing with your patch on top of 8.5.0, the problem is gone and MPD can play the song.

@bubbleguuum
Copy link
Author

bubbleguuum commented Jan 4, 2024

And I confirm patch works with libcurl master as well (without it, this issue results in 100% CPU usage in the mpd io thread).
Thanks for the fix!

@bagder bagder closed this as completed in 8e2d7b9 Jan 4, 2024
gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants