Repository navigation
fix: keep the status of synchronous responses on late client failures - #1608
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A disconnect before the first body byte can hide truncated-response errors from synchronous callers, and the zero-length response test has a connection-pool race that can cause spurious timeouts.
2 open issues
What changed in this PR
This PR addresses #1601 by preserving synchronous HTTP response statuses when the Netty client fails after receiving the response head, building on the diagnostics added in #1602.
Changes:
- Preserves synchronous response statuses on late failures and ends zero-length response streams immediately.
- Identifies connection closures caused by client shutdown with
"Client stopped". - Adds regression tests for late shutdown failures and empty-response connection reuse, plus a changelog entry.
| File | Description |
|---|---|
org.restlet/src/test/java/org/restlet/engine/connector/ClientLateFailureTestCase.java |
Adds raw-socket tests for late failures and connection reuse. |
org.restlet/src/main/java/org/restlet/engine/netty/NettyClientHelper.java |
Marks connections before closing them during shutdown. |
org.restlet/src/main/java/org/restlet/engine/netty/NettyClientHandler.java |
Reports shutdown-specific failures, including through parent channels. |
org.restlet/src/main/java/org/restlet/engine/netty/NettyClientCall.java |
Adjusts late-failure status handling and empty-body completion. |
changes.md |
Documents the client response-handling fix. |
Review effort: Balanced
jlouvel
force-pushed
the
fix/netty-client-late-failure-status
branch
2 times, most recently
from
November 1, 2026 16:18
5e636bb to
f84a105
Compare
A failure after the response head no longer replaces the status returned to a synchronous caller with a communication error (1001): it reaches the response entity stream, and the onResponse callback of asynchronous requests. A response announcing an empty body with "Content-Length: 0" is complete with its head. The failures caused by stopping the client report "Client stopped" instead of "Connection closed by peer". This fixes the intermittent SecurityTestCase failures: the test released the resource, stopping its client, before the end of the error page body. Refs #1601 Co-Authored-By: Claude Opus 5.5 <[email protected]>
A synchronous caller resumes when the response body ends. Ending the body of a response announcing an empty one at its head, or before releasing its connection, let the caller send its next request before the connection was back in the pool, so that request opened a new connection. This made ClientLateFailureTestCase.completesResponsesWithAnEmptyBodyWithTheirHead time out on CI. The body now ends once the response is complete and its connection released, while failures after the head of an empty body are still ignored. Co-Authored-By: Claude Opus 5.5 <[email protected]>
jlouvel
force-pushed
the
fix/netty-client-late-failure-status
branch
from
November 1, 2026 21:29
62e72d6 to
7c725c6
Compare
When the connection failed after a response head announcing a body but before its first byte, the entity was dropped, leaving a synchronous caller with the status and no sign of the truncated response. The failed stream is now kept as the entity, so that reading it reports the failure. A response announcing an empty body still has no entity. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
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
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.




Refs #1601.
Cause
The FINE logs from #1602 captured a failure in CI. In
SecurityTestCase, the client gets the 403 head and the first bytes of the server's HTML error page, soresource.get()returns. The test then callsresource.release(), which stops theClientResource's own client while the rest of the body is still arriving. Closing the connection fails the in-flight call. Since the head was already received,dispatchResponseFailureCallback()then set the status toCONNECTOR_ERROR_COMMUNICATION(1001), even for synchronous requests. That's the sameResponseobject the test reads afterwards, so its 403 turned into 1001.The passing runs show the same order, but there the end of the response arrives before the close.
Changes
NettyClientCall:IOExceptionon read.onResponsecallback.Content-Length: 0(and no chunked encoding), a failure on the connection isn't reported as a response failure.NettyClientHandlerandNettyClientHelper:stop()marks its connections before closing them. A call cut short by the stop now fails withIOException("Client stopped")instead of "Connection closed by peer". For HTTP/2 and HTTP/3 streams, the mark on the parent connection is checked too.ClientLateFailureTestCase(new), with a raw socket server:expected: <Forbidden (403)> but was: <Communication Error (1001)>.Content-Length: 0responses on one connection: both 403 with an empty entity, and the connection is reused.changes.md: entry under Bugs fixed.Http2CancellationTestCasestill passes: it's asynchronous, so its callback and status still get 1001.Validation
./mvnw -B -pl org.restlet -P default,test verify: 2217 tests pass, the coverage check passes, and SpotBugs still reports 8 findings.🤖 Generated with Claude Code