On Tue, 27 Jun 2023 11:02:48 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 107:
>
>> 105: } catch (Exception e) {
>> 106: e.printStackTrace();
>> 107: assertTrue(e.getCause() instanceof IOException, "HTTP/2
>> should cancel with an IOException when the Subscription is cancelled.");
>
> I believe we want to check that the exception we get as the cause is the
> expected CancelException, or at least that it can be found somewhere while
> following the cause chain. Otherwise looks good!
So for HTTP/2, the `cancelImpl()` method in `Stream` looks like this.
@Override
void cancel() {
if ((streamid == 0)) {
cancel(new IOException("Stream cancelled before streamid
assigned"));
} else {
cancel(new IOException("Stream " + streamid + " cancelled"));
}
}
When the new IOException is passed to cancel, it sets the `errorRef` field to
the IOException which then gets returned to us as the cause of the
CompletableFuture's cause for cancelling. I think this behaviour is acceptable
as the IOException returned will say "Stream x cancelled" which is accurate.
Possible that I dont need to have this "CancelException" here
May be more difficult to verify the cause of cancellation beyond that which
I'll think about.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14625#discussion_r1250572721
