RFR [12] 8211437: java.net.http.HttpClient hangs on 204 reply without Content-length 0

[Michael McMahon]

Hi Daniel,

On 18/10/2018, 16:01, Daniel Fuchs wrote:
> Hi Michael,
>
> Thanks for taking on all the feedback!
>
> MultiExchange.java:
>
>  254     if (bodyIsPresent(r)) {
>  255         IOException ioe = new IOException(
>  256             "unexpected content length header with 204 response");
>  257         exch.cancel();
>  258         return MinimalFuture.failedFuture(ioe);
>
> I believe it would be more correct to call
>
>              exch.cancel(ioe);
>
> at line 257 above...
>
Yes, well spotted. That was what was intended there.

Thanks,

Michael

>
> In Response204.java:
>
>   48         Logger logger = Logger.getLogger ("com.sun.net.httpserver");
>   49         ConsoleHandler c = new ConsoleHandler();
>   50         c.setLevel (Level.WARNING);
>   51         logger.addHandler (c);
>   52         logger.setLevel (Level.WARNING);
>
>
> Not that it matters much, but that is strange - I'd understand
> if both Logger and ConsoleHandler were set to something below
> INFO - like e.g. FINE, FINER, FINEST, or ALL?
>
> Otherwise looks good.
>
> IIUC Response204 tests HTTP/1.1 and NoBodyTest tests
> HTTP/2.
>
> Probably not worth it to have a test that tests all 4 flavors
> of HTTP/1.1, HTTPS/1.1, HTTP/2, HTTPS/2 ?
>
> best regards,
>
> -- daniel
>
>
> On 18/10/2018 14:58, Michael McMahon wrote:
>> Updated webrev for this at 
>> http://cr.openjdk.java.net/~michaelm/8211437/webrev.2/index.html
>> based on feedback below.
>>
>> I also made a change to the com.sun httpserver. It changes the recent 
>> fix related to the same
>> issue such that by default the server will not send a content-length 
>> back, if the user explicitly
>> sets one, then it will be sent. This is useful for testing this fix 
>> here.
>>
>> Thanks,
>>
>> Michael
>>
>> On 15/10/2018, 16:07, Michael McMahon wrote:
>>> Hi Daniel,
>>>
>>> On 15/10/2018, 12:53, Daniel Fuchs wrote:
>>>> Hi Michael,
>>>>
>>>> On 15/10/2018 11:54, Michael McMahon wrote:
>>>>> Could I get the following fix reviewed please.
>>>>>
>>>>> http://cr.openjdk.java.net/~michaelm/8211437/webrev.1/index.html
>>>>
>>>> Looks good in general.
>>>>
>>>> In MultiExchange.java:
>>>>
>>>> 236             T nullBody = cf.get();
>>>>
>>>> Though technically the body should be available by the
>>>> time we reach this line, since you completed the subscriber
>>>> just before, we can't really make any assumption on the
>>>> implementation of the subscriber.
>>>>
>>>> So for the sake of robustness we should probably use
>>>> getBody().handle(...) to complete `result`
>>>> rather than calling cf.get();
>>>>
>>>
>>> Yes, that would be better.
>>>
>>>>
>>>> Also I wonder what should happen if a body is present:
>>>>
>>>> Should we simply read it instead?
>>>> Because if we don't then we should close the connection (HTTP/1.1)
>>>> or reset the stream (HTTP/2) - which probably means getting back
>>>> to the concrete  exchange implementation to make sure that
>>>> happens.
>>>>
>>>
>>> I see that as a protocol error. So, rather than attempting to read the
>>> body, I think we should ensure that the request fails (which it does
>>> if a content length or transfer encoding field is present). The 
>>> connection
>>> also needs to be closed (or stream reset) which I need to check is 
>>> being done.
>>>
>>> Thanks,
>>> Michael
>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Michael.
>>>>
>