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