Dialogue response body stream safety #1740
Conversation
It's fully expected that closing the stream will not be able to consume remaining bytes, the connection has been severed. This PR introduces guard rails to prevent attempts to read a closed stream after response.close has been invoked.
Generate changelog in
|
| @@ -455,37 +475,73 @@ public void close() {} | |||
|
|
|||
| private static final class ResponseInputStream extends FilterInputStream { | |||
|
|
|||
| // Client reference is used to prevent premature termination | |||
| @Nullable | |||
| private ApacheHttpClientChannels.CloseableClient client; | |||
There was a problem hiding this comment.
This client ref was unnecessary due to the strong reference to HttpClientResponse response.
| try { | ||
| response.close(); | ||
| } finally { | ||
| super.close(); |
There was a problem hiding this comment.
This super.close() call is unnecessary as it may attempt to drain remaining bytes, which is already handled by response.close(). super.close may throw if response.close chose to terminate the connection rather than fully draining.
| if (snapshot == null) { | ||
| snapshot = createResponseBody(); |
There was a problem hiding this comment.
entity.getContent creates a new state tracking wrapper around the delegate InputStream for each invocation, by holding a reference to the single delegate, we can be more confident that state is consistent.
| testRuntimeOnly 'org.apache.logging.log4j:log4j-slf4j-impl' | ||
| testRuntimeOnly 'org.apache.logging.log4j:log4j-core' |
There was a problem hiding this comment.
With the logging implementation in place, the reported warnings were very obvious in tests.
| } | ||
| } finally { | ||
| client = null; | ||
| public int read() throws IOException { |
There was a problem hiding this comment.
Is there some extra testing we can write to check the behavior you're fixing?
...ache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java
Show resolved
Hide resolved
...ache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java
Show resolved
Hide resolved
...ache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java
Show resolved
Hide resolved
|
Released 3.62.0 |
Before this PR
Warnings logged when the response body was closed before fully consuming content due to a
runtime.discardEndpoint()followed byresponse.close()which internally attempts to consume remaining data from a stream that has been disconnected.After this PR
==COMMIT_MSG==
Dialogue response body stream safety improvements to prevent accidental stream reuse and confusing warnings.
==COMMIT_MSG==