fix(netty): fail fast when InputStream body cannot be reset - #2312
Merged
hyperxpro merged 3 commits intoAug 16, 2026
Merged
Conversation
HTTP/1 path silently returned after a warn when a consumed non-resettable InputStream body was reused, leaving a half-sent request that hung until timeout. Throw IOException so sendHttpRequest aborts the future (matches existing HTTP/2 behavior). Composer 2.5 on behalf of arimu1 Fixes AsyncHttpClient#1973 Co-Authored-By: Composer 2.5 <composer@cursor.com>
Composer 2.5 on behalf of arimu1 Co-Authored-By: Composer 2.5 <composer@cursor.com>
hyperxpro
requested changes
Aug 15, 2026
Unsolicited HTTP/1 100 Continue was replaying bodies that were already sent. Guard with bodyWasDeferred like HTTP/2. Treat a failed reset() on a closed markable stream as unreplayable so retries abort the future instead of hanging or throwing Stream closed. Comments now cite NettyRequestSender.writeRequest. Addresses review on AsyncHttpClient#2312. Composer 2.5 on behalf of arimu1 Co-Authored-By: Composer 2.5 <composer@cursor.com>
Contributor
Author
|
Addressed the CHANGES_REQUESTED review (commit
Happy to follow up if anything still looks off. |
hyperxpro
approved these changes
Aug 16, 2026
Member
|
Thanks a lot! |
4 tasks
4 tasks
hyperxpro
added a commit
that referenced
this pull request
Aug 31, 2026
## Summary - Build keep-body redirects from the original request, preserving every supported body representation and per-request setting. - Clear target-specific routing and credential state when a redirect crosses an origin, including separately stored Cookie objects. - Preserve an explicit `Content-Length` when replaying a raw `InputStream` or an unknown-length `InputStreamBodyGenerator`. - Replay resettable streams and fail promptly for consumed raw streams, streamed multipart parts, and vanished files that cannot be replayed safely. - Pin body bytes, headers, body-selection precedence, caller-owned `ByteBuf` references, and the new failure modes with focused tests. ## Problem `Redirect30xInterceptor` rebuilds a request when it follows a strict 302, 307, or 308 redirect. Its keep-body copy chain handled form parameters, strings, byte arrays, `ByteBuffer`, body generators, and multipart bodies, but omitted four real request send paths: - `List<byte[]>` / composite byte arrays - Netty `ByteBuf` - `InputStream` - `File` The redirected request therefore kept its method and Content-Type but sent zero bytes. The `File` case is especially risky for uploads because the target can accept an apparently valid empty PUT or POST. Reconstructing the request field by field also omitted unrelated per-request state such as the read timeout and range offset, and maintaining a second body-selection chain alongside `NettyRequestFactory` made future drift likely. This is a pre-existing omission. AHC issue [#1643](#1643) previously fixed the same class of bug for multipart bodies. The copy chain was carried through pull request [#1843](#1843) without a policy discussion. Focused searches found no existing issue or pull request covering these four representations. ## Change Build a keep-body redirect with `request.toBuilder()` and then replace only redirect-specific state. This preserves all current and future body representations and per-request options without duplicating `NettyRequestFactory.body`. Headers are copied before redirect-only values are removed, so the original request is not mutated. On a cross-origin redirect, the copied request drops the previous resolved address, virtual host, realm, authorization headers, and Cookie objects before the cookie store adds cookies that legitimately match the new URI. The body is not covered by that boundary. It follows the existing keep-body policy, which means a `File` or `InputStream` body that a cross-origin redirect leg previously received as empty is now sent in full, and a target that keeps redirecting can receive it once per hop up to `maxRedirects`. That is the same exposure byte arrays, strings, form parameters, and multipart bodies already have today. Composite byte arrays, caller-owned `ByteBuf`s, and files are repeatable. A resettable `InputStream`, such as `ByteArrayInputStream`, also replays. A caller-supplied `Content-Length` is retained for a raw `InputStream` or an `InputStreamBodyGenerator` without a declared length, because neither has an intrinsic size from which to recompute it. A consumed stream that cannot be reset reaches the existing fail-fast guard added in #2312 and completes the future with `IOException`; that is preferable to silently succeeding with an empty body. An `InputStreamPart` is closed by the first multipart send and has no equivalent replay guard, so a keep-body redirect now fails promptly instead of risking a hang or incomplete multipart request. A selected `File` or `FileBodyGenerator` is also checked before dispatching the redirect; if it disappeared after the first send, the future fails with `IOException` before a target pooled channel can be removed and an unchecked constructor exception can escape. The validation follows `NettyRequestFactory` precedence so a sticky `File` field is ignored when a higher-priority body representation was actually sent. The change does not alter which methods or status codes keep a body, nor does it introduce a new cross-origin policy. It makes the existing strict-302, 307, and 308 behavior complete for every supported request-body representation. ## Compatibility There is no public API change. Requests that previously sent an empty body on a keep-body redirect now resend their configured body. **Behavior changes:** - A non-resettable `InputStream` on a keep-body redirect previously completed successfully after sending an empty redirected request. It now completes the request future exceptionally with `IOException`. This includes `FileInputStream`, which is closed after the first send and cannot be reset for replay. - A multipart `InputStreamPart` now fails promptly with `IOException` when a keep-body redirect requires replay. Reusing its already-consumed and closed stream could previously hang or send incomplete multipart content. - A selected file that disappears between the first request and redirect now fails with `IOException` before redirect dispatch rather than allowing an unchecked `IllegalArgumentException` to escape while constructing the next request. - A `File` or `InputStream` body is now sent on a keep-body redirect to a different origin, where the redirected request previously carried no body. Credentials are still stripped at that boundary, but the payload is not. Callers that accidentally relied on an empty or incomplete redirected request will observe an exception, but the failure is explicit instead of silently losing configured content. There is no public API change. ## AI disclosure OpenAI Codex on behalf of Matthias Kurz. The commit includes `Co-Authored-By: OpenAI Codex <codex@openai.com>` per `AGENTS.md`. ## Test plan - [x] On untouched `upstream/main`, the focused suite reproduced five failures: four body types arrived as zero bytes and a non-resettable stream incorrectly completed successfully. - [x] Before the generator fix, a one-argument `InputStreamBodyGenerator` sent `Content-Length: 13` on the first leg and no `Content-Length` on the redirected leg. - [x] `./mvnw -pl client -Dtest=RedirectBodyTest,RedirectCredentialSecurityTest test` on JDK 11: 40 tests passed, including Netty leak detection. - [x] `./mvnw clean verify` on JDK 11: 1,496 tests passed and Revapi completed without failures (`BUILD SUCCESS`). Generated with OpenAI Codex. --------- Co-authored-by: OpenAI Codex <codex@openai.com> Co-authored-by: Aayush Atharva <24762260+hyperxpro@users.noreply.github.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
InputStreamrequest body used to log a warning and return, leavingNettyRequestSender.writeRequestwithout completing the future so the request hung until timeout (Failed stream request hangs until timeout #1973). Headers usedchannel.writewith no flush; the peer has seen nothing.IOExceptioninstead sowriteRequestaborts the future. A failedreset()on a closed markable stream (for exampleBufferedInputStreamafterWriteProgressListenercloses it) is treated the same way, so retries fail fast instead of hanging or throwing only "Stream closed".bodyWasDeferredguard as HTTP/2, so an unsolicited 100 (RFC 9110 15.2.1) does not replay a body that was already sent.write,writeHttp2, thewriteRequestabort path, and a real request replay.Fixes #1973
AI disclosure
Composer 2.5 on behalf of arimu1 (Cursor harness). Commit includes
Co-Authored-By: Composer 2.5perAGENTS.md.Test plan
./mvnw -pl client -Dtest=NettyInputStreamBodyTest,Continue100InterceptorTest,NettyRequestSenderConsumedBodyRetryTest test(JDK 17)Made with Cursor