armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Ensure HTTP/2 flow control to work at the stream level

Open ikhoon opened this issue 6 months ago • 3 comments

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 InboundTrafficController with HTTP/2 Http2LocalFlowController
    • Invokes Http2LocalFlowController.consumeBytes() when steam data is consumed to send a WINDOW_UPDATE frame.
  • Introduced {ServerBuilder,ClientFactoryBuilder}.http2StreamWindowUpdateRatio to customize the threshold to send WINDOW_UPDATE frames.
  • Fixed client.Http2ResponseDecoder and server.Http2RequestDecoder to defer reporting the consumed data length.
    • The length is not reported via InboundTrafficController when the data is consumed in userland.

Result:

  • Fixed a bug where HTTP/2 flow control did not work properly, and stream-level windowing was ignored.
  • Closes #6253

ikhoon avatar Jun 04 '25 13:06 ikhoon

🔍 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

github-actions[bot] avatar Jun 04 '25 14:06 github-actions[bot]

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.

Files with missing lines Patch % Lines
...eria/internal/common/InboundTrafficController.java 78.57% 3 Missing and 3 partials :warning:
...c/main/java/com/linecorp/armeria/common/Flags.java 75.00% 1 Missing :warning:
...p/armeria/internal/client/DecodedHttpResponse.java 83.33% 0 Missing and 1 partial :warning:
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.

codecov[bot] avatar Jun 05 '25 07:06 codecov[bot]

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.

jrhee17 avatar Jun 10 '25 01:06 jrhee17

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.

ikhoon avatar Jun 20 '25 02:06 ikhoon

hi @trustin could you review it, thanks!

yzfeng2020 avatar Jul 11 '25 16:07 yzfeng2020