RFR: 8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient

[Conor Cleary]

On Tue, 27 Jun 2023 11:02:48 GMT, Daniel Fuchs <dfuchs at openjdk.org> 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