RFR:8212114 Reconsider the affect on closed streams resulting from 8189366
Hi All, Please review the below fix. Webrev: http://cr.openjdk.java.net/~vtewari/8212114/webrev0.0/index.html bugId: https://bugs.openjdk.java.net/browse/JDK-8212114 this change the revert the effect of JDK-8189366, restore the existing(SocketInputStream:available() will throw the socketException if the underline socket is closed.) long-standing behavior. Thanks, Vyom
Re: RFR [12] 8211437: java.net.http.HttpClient hangs on 204 reply without Content-length 0
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.
Re: RFR [12] 8211437: java.net.http.HttpClient hangs on 204 reply without Content-length 0
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.
Re: RFR [12] 8211437: java.net.http.HttpClient hangs on 204 reply without Content-length 0
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.
