On Tue, 4 Jul 2023 07:40:59 GMT, Daniel Fuchs <[email protected]> wrote:
>> **Issue** >> In CancelledResponse.java the test only checks the HttpClient against >> HTTP/1.1 when cancelling a BodySubscriber while receiving data. >> >> **Solution** >> In the interest of more coverage, a new test has been created which performs >> the same checks against HTTP/2 to cover the client in the case of a HTTP/2 >> connection. A new test was created as it makes use of HttpTestServerAdapters >> to create the test servers. This is different to how this is performed in >> the original "CancelledResponse" test. There are some minor changes to how >> the testing is conducted with an element of randomness added to the new test. >> >> As an open question to reviewers, the old test "CancelledResponse" and the >> new test "CancelledResponse2" could be merged into a single file and the >> HTTP/1.1 case could be updated to use more canonical testing methods as with >> "CancelledResponse2". Though there isn't a very pressing need for this and >> so it has not been included in this PR as of now. > > test/jdk/java/net/httpclient/CancelledResponse2.java line 228: > >> 226: cancelled.set(true); >> 227: subscription.cancel(); >> 228: result.completeExceptionally(new CancelException()); > > I believe that's the exception we expect to find in the `HttpResponse`. IIRC > the original test was checking for that. Right, but calling `subscription.cancel()` causes `Stream.cancelImpl()` to be called. That in turn causes the `HttpResponse` (or the variable result in your snippet above) to complete exceptionally with an IOException which has the message "Stream x cancelled". I think that means that the call to `completeExceptionally(new CancelException())` has no effect because `subscription.cancel()` triggers a call to `cancelImpl()` ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14625#discussion_r1251778369
