Ensure HTTP/2 flow control to work at the stream level
Motivation:
Since InboundTrafficController operates at the TCP level, it isn't aligned with HTTP/2 flow control. As a result, a stream may receive more than its stream window size allows and interfering with other streams from receiving data.
Related: #6253
Modifications:
- Integrated
InboundTrafficControllerwith HTTP/2Http2LocalFlowController- Invokes
Http2LocalFlowController.consumeBytes()when steam data is consumed to send aWINDOW_UPDATEframe.
- Invokes
- Introduced
{ServerBuilder,ClientFactoryBuilder}.http2StreamWindowUpdateRatioto customize the threshold to send WINDOW_UPDATE frames. - Fixed
client.Http2ResponseDecoderandserver.Http2RequestDecoderto defer reporting the consumed data length.- The length is not reported via
InboundTrafficControllerwhen the data is consumed in userland.
- The length is not reported via
Result:
- Fixed a bug where HTTP/2 flow control did not work properly, and stream-level windowing was ignored.
- Closes #6253
🔍 Build Scan® (commit: 114f2271eaf06d8443d55527326e0e97b69967b0)
| Job name | Status | Build Scan® |
|---|---|---|
| build-ubicloud-standard-16-jdk-8 | ✅ | https://ge.armeria.dev/s/dfxbo2ipzzbu4 |
| build-ubicloud-standard-16-jdk-21-snapshot-blockhound | ❌ (failure) | https://ge.armeria.dev/s/tw6p6vl7w6szm |
| build-ubicloud-standard-16-jdk-17-min-java-17-coverage | ✅ | https://ge.armeria.dev/s/odxp2jmyuii32 |
| build-ubicloud-standard-16-jdk-17-min-java-11 | ✅ | https://ge.armeria.dev/s/c2kjmoe5whkkm |
| build-ubicloud-standard-16-jdk-17-leak | ✅ | https://ge.armeria.dev/s/d5ynryxhzxloy |
| build-ubicloud-standard-16-jdk-11 | ✅ | https://ge.armeria.dev/s/tpnuqt6xfbnv6 |
| build-macos-latest-jdk-21 | ✅ | https://ge.armeria.dev/s/lasp5u6yjr55m |
Codecov Report
Attention: Patch coverage is 91.11111% with 8 lines in your changes missing coverage. Please review.
Project coverage is 74.57%. Comparing base (
8150425) to head (114f227). Report is 125 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #6266 +/- ##
============================================
+ Coverage 74.46% 74.57% +0.11%
- Complexity 22234 22476 +242
============================================
Files 1963 1985 +22
Lines 82437 83079 +642
Branches 10764 10787 +23
============================================
+ Hits 61385 61957 +572
- Misses 15918 15967 +49
- Partials 5134 5155 +21
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Note: I understood that if users are not interested in the server request or client response (fire-and-forget pattern), then the window frame won't be sent. ref: https://github.com/jrhee17/armeria/tree/poc/fire-and-forget
I think most users won't be using this pattern since, but something to keep in mind in case someone asks about this behavior after applying this patch.
if users are not interested in the server request or client response (fire-and-forget pattern)
From what I understand, Armeria requires users to consume all HttpResponses to avoid memory leaks.
hi @trustin could you review it, thanks!