Skip to content

HTTPCLIENT-2433: Prevent concurrent HTTP/2 request body replay - #884

Open
mkurz wants to merge 2 commits into
apache:masterfrom
mkurz:HTTPCLIENT-2433
Open

mkurz wants to merge 2 commits into
apache:masterfrom
mkurz:HTTPCLIENT-2433

Conversation

@mkurz

@mkurz mkurz commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

This fixes HTTPCLIENT-2433.

When an HTTP/2 authentication challenge completed before the request body had finished, H2AsyncMainClientExec previously notified the execution chain that the exchange was complete immediately. The authentication handler could then release and reuse the same repeatable entity producer while the original HTTP/2 stream was still consuming it, corrupting the replayed request body.

The HTTP/2 execution handler now completes the exchange only after both request output and response input have terminated. It also handles a graceful RST_STREAM(NO_ERROR) after a complete response as request-side termination, allowing authentication replay to continue without hanging.

Regression coverage includes:

  • Uploading a deterministic 2 MB file through challenge-based Basic authentication and verifying that the replay contains the exact original bytes.
  • A body-less 401 followed by RST_STREAM(NO_ERROR), verifying that the authenticated retry completes with the exact 2 MB request body.

Reproducer:
https://github.com/mkurz/apache-httpclient5-h2-auth-replay-reproducer

Tests:

  • HTTP/2 authentication suites with and without TLS: 28 tests, 0 failures
  • Graceful-reset regression test repeatedly verified with and without TLS

ok2c and others added 2 commits September 16, 2026 19:09
…cuted manually during release preparation due to those tests failing intermittently in CI
Wait for both sides of an HTTP/2 exchange to terminate before completing it, including graceful request termination after RST_STREAM(NO_ERROR).

Cover authentication replay with a large file and with a body-less 401 followed by a graceful stream reset.
@ok2c

ok2c commented Sep 17, 2026

Copy link
Copy Markdown
Member

@mkurz The test coverage looks awesome! However I need a little while to digest the proposed fix. Please bear with me.

@ok2c ok2c left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mkurz Your suggested fix is perfectly fine.

For the sake of code consistency could we use the same approach as used in HttpAsyncMainClientExec using AtomicInteger instead of two AtomicBoolean?

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