Fix for stalled streams #7801
Fix for stalled streams #7801
Conversation
okhttp/src/jvmMain/kotlin/okhttp3/internal/http2/Http2Stream.kt
Outdated
Show resolved
Hide resolved
| @@ -415,8 +416,6 @@ class Http2Stream internal constructor( | |||
| } | |||
|
|
|||
| if (readBytesDelivered != -1L) { | |||
| // Update connection.unacknowledgedBytesRead outside the synchronized block. | |||
| updateConnectionFlowControl(readBytesDelivered) | |||
There was a problem hiding this comment.
Ooooh changes like this make me want to double- or triple- check our tests
|
I don’t think our tests are sufficient to give much confidence that a change like this is safe, or that it’s unsafe. We have tests to confirm book-keeping happens, but not very much! I think we need more aggressive tests here if we’re going to change this policy. I’d like to explore other strategies here. I believe the current implementation implements a specific policy: acknowledge received bytes whenever we’ve consumed 50% of the window. I think we just need a different policy; perhaps limiting on streams only and never limiting on the connection. |
|
I don't think the connection window can be int.max, it would swamp devices with a firehouse of data from fast servers. And my change is sort of doing what you suggest. Ensuring connection credits are bumped as data comes in, but we wait until the readers consumes stream data before bumping those bytes. So you want this pulled out into some understandable/testable FlowStrategy? As is I'd like to create tests to confirm existing and new behaviour which should make our policy clear. |
|
@swankjesse I think we agreed over chat, not to complicate this with a more advanced or tunable implementation. You seemed also fine with the expected memory being higher in apps with stalled streams. I'd like to land shortly after #7810 and get a snapshot out to test with media3. |
(cherry picked from commit b11a6a8)
Moves updating the connection flow control window from consuming (read) to receiving from the connection (receive).
Avoids any stalled reader from stopping other streams, which is currently very likely.
However it increases the max (not typical) memory required to Stream Count * 16MB, instead of current 16MB.