-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
OkHttp HTTP/2 2nd request being blocked #7768
Comments
I suspect it's head of line blocking related to this scenario. androidx/media#188 HTTP/2 is over a multiplexed socket, so if one request has data, but is not being read, it can block other requests. OkHttp doesn't handle this at all, but it's possible there are some improvements in either Media3/ExoPlayer or OkHttp that could make this less likely. For now, streaming over Http/1.1 is probably the safest option. |
As an extra bit of info, if you switch @yoobi's demo to use Media3's |
Thanks, I'll take a look in the next week or so. Hopefully a nice fix, we can do to improve OkHttp. |
I've reproduced with your repo and an emulator, and tried reproducing in a standalone JVM project without success. So trying to get the right logging in place to understand what's going on in the emulator. |
I've added some additional debug in https://github.com/yschimke/ExoPlayer/commit/a47acac432c752d621de48ed8a97654425d3896e Generally following https://square.github.io/okhttp/contribute/debug_logging/, but there the tags are different than I expected. You need to enable the additional logging with
There are more requests than I expect initially on streaming. It looks like one is being cancelled early on, and the server is continuing to send data. Possibly similar to a previous bug with thread interrupts, that I've fixed in one place. But still working through it.
We are ending up with a ton of writeSynReset calls, which suggest we think this stream is dead, but getting flooded by the traffic in the meantime. But I'm guessing at the moment. |
The 3 calls we see up to the first download request (the 4th).
|
I think I have a repro of the problem in a JVM. but I'll discuss the exact flow of requests with @icbaker tomorrow, because it seems like ExoPlayer is opening 3 streams and leaving them open while playing the video, and OkHttp doesn't seem to nicely handle 4 open streams in this case with only the 4th being consumed. |
The 3 requests associated with the playback (before the download one) are explained for this piece of content as follows:
This jumping around is due to the structure of this MP4 file - see more discussion/details here: google/ExoPlayer#6635 However I am surprised that you see all 3 of these requests remain open. I would expect each to be closed before the next is made, and only the third would remain open while the video was paused. I added some very quick logging to the demo app and saw the It's not obvious to me if '4' specifically is a problem here (and so if there were only 2 concurrent requests things would work) - if that's the case then it seems we can just work out how to make sure those first two requests are closed fully/properly. However maybe there would still be this problem with 2 concurrent requests (though ofc it would still be good to understand why those other 2 aren't being closed). |
My repro cancels 1 & 2, so only 3 stays live, and we see 4 still blocks. So it matches what you are explaining. I don't see a cancel for them, presumably because 1 & 2 are via thread interrupts, or 2 completes normally?. Which can be problematic, but should work in this case. We continue to get data for 1 & 2 because the roundtrip allows for a lot of extra data in the meantime. I don't think ExoPlayer is doing anything wrong, so the fix is probably in OkHttp. @swankjesse suggested we might have a strong false assumption that readers read :) |
I've added in the OkHttp Wireshark sample. I can confirm that OkHttp isn't sending a Window Update for the stream that isn't being read. So I'll debug through that next.
|
I think this is a possible fix #7801, but with low confidence it doesn't break something, or is undesirable. |
Hoping to land both of those soon and have a published snapshot version of 5.x to test with. Will confirm with that repro. |
This is no longer reproducible on the exoplayer demo using a 5.0.0-SNAPSHOT, the download completes quickly and successfully. I'd like to wait for a 5.0.0-alpha release gets some real use, before looking to backport to a 4.12. So it's probably a small number of weeks for a 5 alpha, and couple of months for a 4.12? But that's a guess. to test, change to version 5.0.0-SNAPSHOT and add this repository
|
Fix coming in 5, hopefully to land in a 4.12 at some point. |
Any forecast when this will land in a 5.0-alpha or (even better) in a 4.12? |
@yschimke do you plan to release 4.12? |
Released |
Hello,
I'm not sure how to explain it more clearly than what ian said so I'll quote from the original issue
To reproduce the issue you can checkout my branch: https://github.com/yoobi/ExoPlayer/tree/issue/11120 and do the following:
PlayerActivity
)You'll see the download is stuck at the beginning while you're on the PlayerActivity. Once you leave the
PlayerActivity
, the download is "resumed" and progress smoothlyit looks to be somewhat related to #7764 but I'm not entirely sure
The text was updated successfully, but these errors were encountered: