On Mon, 3 Jul 2023 09:39:13 GMT, Conor Cleary <[email protected]> wrote:
>> 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.
There is a check that the cancelling BodyHandler sets the cancelled field which
may be sufficient
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14625#discussion_r1250574856
