Skip to content

Fix HTTP2 connections with multiple streams stalling#193

Merged
patrickbkr merged 1 commit intocroservices:masterfrom
patrickbkr:http2-multi-stream-window-updates
Aug 9, 2024
Merged

Fix HTTP2 connections with multiple streams stalling#193
patrickbkr merged 1 commit intocroservices:masterfrom
patrickbkr:http2-multi-stream-window-updates

Conversation

@patrickbkr
Copy link
Member

HTTP2 knows a stream and connection flow-control window. Both windows need to be respected. A logic was put in place, to not update the window sizes when the stream that sent the data was closed. That's fine. But we do need to update the global window, otherwise the global window size permanently shrinks by that amount. When reusing a connection many times, this shinks the window size down to zero over time, effectively stalling the connection.

HTTP2 knows a stream and connection flow-control window. Both windows need
to be respected. A logic was put in place, to not update the window sizes
when the stream that sent the data was closed. That's fine. But we do need
to update the global window, otherwise the global window size permanently
shrinks by that amount. When reusing a connection many times, this shinks
the window size down to zero over time, effectively stalling the
connection.
@patrickbkr patrickbkr force-pushed the http2-multi-stream-window-updates branch from 28e65f2 to d98aca1 Compare July 23, 2024 07:46
Copy link
Contributor

@peelle peelle left a comment

Choose a reason for hiding this comment

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

Looks good.

@patrickbkr
Copy link
Member Author

Cool!

@patrickbkr patrickbkr merged commit 39022ce into croservices:master Aug 9, 2024
@patrickbkr patrickbkr deleted the http2-multi-stream-window-updates branch August 9, 2024 10:24
@patrickbkr
Copy link
Member Author

patrickbkr commented Aug 9, 2024

@peelle Are you OK with me giving you a commit bit in the croservices org? - Update: I've sent an invite.

@peelle
Copy link
Contributor

peelle commented Aug 10, 2024

@peelle Are you OK with me giving you a commit bit in the croservices org? - Update: I've sent an invite.

Thank you for the invite. I've accepted. I'm excited to help y'all out.

@bduggan
Copy link
Contributor

bduggan commented May 14, 2025

Hi folks -- I've been having occasional issues like the one below with recent versions of cro-http:

    Died with the exception:
        PROTOCOL_ERROR
          in block  at /Users/bduggan/cro-http/lib/Cro/HTTP2/GeneralParser.rakumod (Cro::HTTP2::GeneralParser) line 194
          in block  at /Users/bduggan/cro-http/lib/Cro/HTTP2/FrameParser.rakumod (Cro::HTTP2::FrameParser) line 102
          in block  at /Users/bduggan/cro-http/lib/Cro/HTTP2/FrameParser.rakumod (Cro::HTTP2::FrameParser) line 61
          in block  at /Users/bduggan/.rakubrew/versions/moar-2024.12/share/perl6/site/sources/ABAF64BE62A481E7D08FE8DF38CAF35183A7DB16 (Cro::TLS) line 92

I bisected the changes and ended up at this PR. I'm not sure what the underlying cause is, but commenting out the code that was introduced in this PR resolves the issue. Any ideas?

@patrickbkr
Copy link
Member Author

@bduggan Thanks for digging into this!
Do you see this on the client or server side? Could you try creating a reproduction for this?

@bduggan
Copy link
Contributor

bduggan commented May 15, 2025

This was on the client side. I'll see if I can make a minimal test case or generate a trace log.

@bduggan
Copy link
Contributor

bduggan commented Jul 9, 2025

okay, here's a minimal test case and trace log -> #203

bduggan added a commit to bduggan/cro-http that referenced this pull request Jul 10, 2025
This has a specific meaning that may not be intended

see croservices#193

https://medium.com/coderscorner/http-2-flow-control-77e54f7fd518

WINDOW_UPDATE frame with a value 0 results in a stream error of type protocol error.
@bduggan bduggan mentioned this pull request Jul 10, 2025
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.

3 participants