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

[Daniel Fuchs]

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...


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.
>>>