On Wed, 4 Oct 2023 17:14:01 GMT, Daniel Fuchs <[email protected]> wrote:
> The `AbstractThrowingSubscribers.java` test class makes the assumption that > the HttpClient will cancel a request if the body `CompletableFuture` returned > by a `BodySubscriber::getBody` is completed exceptionally. This assumption is > wrong. Indeed, it should be the responsibility of the custom subscriber to > cancel its subscription, or not, in such a case, as whether to cancel or not > depends on the semantic the subscriber assigns to such a body. > > The issue can be observed by modifying the test to send a longer response > split in several chunks: with such a configuration, and using streaming body > subscribers, such as `ofInputStream`, the test will fail waiting for the > HttpClient to be garbaged collected, as the response data will not get pulled > and the underlying exchange will never terminate. Though changing the > HttpClient code to forcefully cancel the subscription (or the exchange) in > such a case would "work", it does not seem desirable: it would introduce a > change of behavior that might cause existing applications to fail, and there > may be cases where a custom subscriber may legitimately want to complete its > body CF exceptionally while still continuing to parse the body bytes. Whether > to choose to cancel the subscription or not should solely depend on the logic > of the custom subscriber. > > The test logic is now modified to reflect this. Commenting the line that > cancels the subscription in `ThrowingBodySubscriber::getBody` would now make > the test fail. The test was previously passing "by chance" because the whole > response was contained in a single `DataFrame` (or `ByteBuffer` for HTTP/1.1). This pull request has now been integrated. Changeset: 4c5b66dc Author: Daniel Fuchs <[email protected]> URL: https://git.openjdk.org/jdk/commit/4c5b66dceab15ce27f742c4173e14156249eb61a Stats: 82 lines in 1 file changed: 55 ins; 11 del; 16 mod 8317522: Test logic for BODY_CF in AbstractThrowingSubscribers.java is wrong Reviewed-by: djelinski ------------- PR: https://git.openjdk.org/jdk/pull/16041
