HTTP/2 multiplex: Correctly process buffered inbound data even if aut…#9389
HTTP/2 multiplex: Correctly process buffered inbound data even if aut…#9389normanmaurer merged 1 commit into4.1from
Conversation
…oRead is false Motivation: When using the HTTP/2 multiplex implementation we need to ensure we correctly drain the buffered inbound data even if the RecvByteBufallocator.Handle tells us to stop reading in between. Modifications: Correctly loop through the buffered inbound data until the user does stop to request from it. Result: Fixes #9387. Co-authored-by: Bryce Anderson <banderson@twitter.com>
|
I'm testing this patch now. I'll let you know how it goes. I have another idea that I think we should consider: perhaps we should change the behavior of the stream channels to always eagerly push messages down the pipeline and use |
bryce-anderson
left a comment
There was a problem hiding this comment.
It looks like it works locally. I'm a little worried that we can get into a readStatus = REQUESTED state from the fireChildRead method but it appears to be benign.
|
@bryce-anderson i „think“ it should be not a problem, hopefully I am right tho... I also like your idea about pushing eagerly. Maybe we should merge this PR and the investigate with a followup ? |
|
Agreed on merging this and investigating the eager-pushing lazy acking strategy. 👍 |
…ckpressure via window update frames by read() Motivation: At the moment each Http2StreamChannel instance has a complicated state machine to handle auto read configurations. We can simplify this by just always fire frames to the child channel pipelines as soon as we receive them but only give back bytes to the flowcontroller when read() is called. This will ensure that backpressure still is correctly managed via flow control (by window update frames) but simplify things a lot and may release memory more quickly. Fore more details see #9389 (comment). Special thanks to @bryce-anderson for the idea. Modifications: - Remove all the complex buffering logic from AbstractHttp2StreamChannel - Move logic to give back bytes to the flow-controller (and so produce window update frames) to beginRead(...) - Update tests Result: Less complex implementation while still maintain ability to flow control channels.
|
@bryce-anderson PTAL #9390 for an implementation of your idea. |
…oRead is false (#9389) Motivation: When using the HTTP/2 multiplex implementation we need to ensure we correctly drain the buffered inbound data even if the RecvByteBufallocator.Handle tells us to stop reading in between. Modifications: Correctly loop through the buffered inbound data until the user does stop to request from it. Result: Fixes #9387. Co-authored-by: Bryce Anderson <banderson@twitter.com>
…ckpressure via window update frames by read() Motivation: At the moment each Http2StreamChannel instance has a complicated state machine to handle auto read configurations. We can simplify this by just always fire frames to the child channel pipelines as soon as we receive them but only give back bytes to the flowcontroller when read() is called. This will ensure that backpressure still is correctly managed via flow control (by window update frames) but simplify things a lot and may release memory more quickly. Fore more details see #9389 (comment). Special thanks to @bryce-anderson for the idea. Modifications: - Remove all the complex buffering logic from AbstractHttp2StreamChannel - Move logic to give back bytes to the flow-controller (and so produce window update frames) to beginRead(...) - Update tests Result: Less complex implementation while still maintain ability to flow control channels.
…oRead is false
Motivation:
When using the HTTP/2 multiplex implementation we need to ensure we correctly drain the buffered inbound data even if the RecvByteBufallocator.Handle tells us to stop reading in between.
Modifications:
Correctly loop through the buffered inbound data until the user does stop to request from it.
Result:
Fixes #9387.
Co-authored-by: Bryce Anderson banderson@twitter.com