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

Fix for stalled streams #7801

Merged
merged 5 commits into from
May 10, 2023
Merged

Fix for stalled streams #7801

merged 5 commits into from
May 10, 2023

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented May 5, 2023

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.

@@ -415,8 +416,6 @@ class Http2Stream internal constructor(
}

if (readBytesDelivered != -1L) {
// Update connection.unacknowledgedBytesRead outside the synchronized block.
updateConnectionFlowControl(readBytesDelivered)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooooh changes like this make me want to double- or triple- check our tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's next.

@swankjesse
Copy link
Collaborator

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.

@yschimke
Copy link
Collaborator Author

yschimke commented May 6, 2023

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.

@yschimke yschimke marked this pull request as ready for review May 10, 2023 07:51
@yschimke
Copy link
Collaborator Author

@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.

@yschimke yschimke changed the title Attempt a fix for stalled streams Fix for stalled streams May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants