8245462: HttpClient send throws InterruptedException when interrupted but does not cancel request

[Michael McMahon]

Daniel,

That's all fine. Concerning the test, I think the approach looks good,
but I wonder if instead of just synchronizing on the CFs to make
the cancel happen at the same time always, would it be useful to have a 
test mode
where the cancel occurs after different lengths of time to try and exercise
different code paths depending on when exactly it happens?

Michael.

On 23/07/2020 19:13, Daniel Fuchs wrote:
> Hi Michael,
>
> On 23/07/2020 18:48, Michael McMahon wrote:
>> Hi Daniel,
>>
>> This looks like good work. A couple of points/questions:
>>
>> - commented code in RequestPublishers.java can be deleted presumably
>
> Yes - good point.
>
>> - RequestPublishers.IterablePublisher :: computeLength(). What is the 
>> reason for returning -1
>>     here instead of the computed length.
>
> While writing my test - I noticed that my Iterable was called twice.
> Once for computing the length;
> Once for sending the data.
>
> If fetching the data is an expensive operation, then you will
> pay that cost twice. So my reasoning here is that since you
> provide an Iterable - then it should remain lazy, and not
> be iterated twice.
>
> Returning -1 has that effect. It means that Content-Length
> will not be sent for HTTP/2 (which is allowed - as the
> client MAY send Content-Length - while Content-Length is
> not really needed for HTTP/2) and that HTTP/1 will use
> Chunk encoding to send the body.
>
>> - why does Stream.registerStream now take a boolean 
>> 'registerIfCancelled'? In particular why set that to true?
>
> In case where the operation is canceled before the streamId
> is allocated, then there's no reason to actually register the
> stream.
>
> Except in the case where the this is the first stream in an
> upgraded connection - because the headers have already been
> sent - so the stream already de facto exists.
>
> Also I was unsure of the effect of not registering the
> stream for push promises - things are a bit different
> for server initiated streams.
>
> The entire idea is to avoid unnecessary background activity
> (like e.g. sending windows updates or whatever) after the
> operation has been canceled.
>
>> - Do you think the spec change is required normatively? Or could we 
>> do without it, or just add
>>     informative text instead?
>
> I followed Alan's suggestions on this.
>
>>
>> I'm still looking at the test.
>
> Thanks for the feedback!
>
> best regards,
>
> -- daniel
>
>>
>> Thanks,
>>
>> Michael.
>>
>> On 23/06/2020 18:47, Daniel Fuchs wrote:
>>> Hi,
>>>
>>> Please find below a fix for:
>>> 8245462: HttpClient send throws InterruptedException when
>>>          interrupted but does not cancel request
>>> https://bugs.openjdk.java.net/browse/JDK-8245462
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8245462/webrev.00/index.html
>>>
>