RFR [8203036]: com.sun.net.httpserver.HttpExchange should implement AutoCloseable

[Patrick Concannon]

Hi,


Would it be possible to have my fix, and corresponding CSR, for 
JDK-8203036 reviewed?


This fix addresses the retrofitting of HttpExchange to implement 
AutoCloseable so that it can be used with try-with-resources statements.


The CSR for the fix can be found here: 
https://bugs.openjdk.java.net/browse/JDK-8229235
Further information on this bug can be found here: 
https://bugs.openjdk.java.net/browse/JDK-8203036


Webrev for fix: 
http://cr.openjdk.java.net/~michaelm/pconcannon/8229235/webrev.1/



Kind regards,

Patrick

Re: RFR: 8185898: setRequestProperty(key, null) results in HTTP header without colon in request

[Julia Boes]

Hi,

Please find an alternative fix below. Chris and Daniel suggested to add 
a new method MessageHeader::isRequestline rather than a separate data 
field for the requestline. A whitebox test was added with the method 
testMessageHeader.


Updated webrev: http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.4/

Regards,

Julia

On 06/08/2019 11:03, Julia Boes wrote:

Hi Michael and Daniel,

Thanks for your comments. I made the following changes to B8185898:

- Added checks for URLConnection::getRequestProperties, 
::getHeaderField(0), ::getHeaderFieldKey(0)


- Included a test of MessageHeader::print and ::toString. Added a null 
check for the requestLine in MessageHeader::toString (Thanks Jaikiran!)


Updated webrev: 
http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.2/


Best,

Julia


On 31/07/2019 15:34, Daniel Fuchs wrote:

Hi Michael,

On 31/07/2019 14:47, Michael McMahon wrote:

Daniel

I don't think the change affects the usage of response headers,
but it might be useful to include in the test a call to 
getHeaderField(0)

to verify that.


Oh - right - but it will/might affect the toString() of the
response headers, if that is used for logging, isn't it?
Wouldn't the status line now be printed with an additional colon
at the end?

best,

-- daniel



Michael.

On 31/07/2019, 12:30, Daniel Fuchs wrote:

Hi Julia,

Could you verify that `HttpURLConnection::getHeaderField(0)` and
`HttpURLConnection::getHeaderFieldKey(0)` return the same thing
before and after the fix, for all implementations of
`HttpURLConnection` in the JDK?

I believe your test is missing that.

More specifically, I'm referring to this:
https://download.java.net/java/early_access/jdk13/docs/api/java.base/java/net/HttpURLConnection.html#getHeaderFieldKey(int) 



"Some implementations may treat the 0th header field as special,
i.e. as the status line returned by the HTTP server. In this case,
getHeaderField(0) returns the status line, but getHeaderFieldKey(0)
returns null."

I wonder if the JDK itself was one of those implementations before
the fix. We do not want to change the behaviour of these methods.

best regards,

-- daniel

On 31/07/2019 11:56, Julia Boes wrote:

Hi,

Please find below a patch for:

8185898: setRequestProperty(key, null) results in HTTP header 
without colon in request


https://bugs.openjdk.java.net/browse/JDK-8185898

According to RFC 2616 
, message headers 
of a HTTP message must adhere to the format:


field-name ":" [ field-value ]

Previously, the request line was handled like a message header and 
MessageHeader::print would omit ":" for message headers with value 
== null to account for the request line. However, this can result 
in a regular message header missing the colon if its value is 
null. To fix this, MessageHeader.requestLine was added, which is 
used only for the status line of a request. Any use of 
MessageHeader.set(0) and MessageHeader.prepend() in 
HttpURLConnection was replaced by MessageHeader::setRequestLine.


Webrev:

http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.1/


Cheers,

Julia

Re: RFR: 8185898: setRequestProperty(key, null) results in HTTP header without colon in request

[Daniel Fuchs]

Hi Julia,

On 09/08/2019 13:40, Julia Boes wrote:

Hi,

Please find an alternative fix below. Chris and Daniel suggested to add 
a new method MessageHeader::isRequestline rather than a separate data 
field for the requestline. A whitebox test was added with the method 
testMessageHeader.


Updated webrev: 
http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.4/


Thanks for doing this! This new approach seems safer to
me as it's less likely to cause compatibility issue with
code that might be using MessageHeader elsewhere.

Make sure to test thoroughly though! :-)

best regards,

-- daniel



Regards,

Julia

Re: RFR: 8185898: setRequestProperty(key, null) results in HTTP header without colon in request

[Chris Hegarty]

Julia,

> On 9 Aug 2019, at 13:40, Julia Boes  wrote:
> 
> Hi,
> 
> Please find an alternative fix below. Chris and Daniel suggested to add a new 
> method MessageHeader::isRequestline rather than a separate data field for the 
> requestline. A whitebox test was added with the method testMessageHeader.
> 
> Updated webrev: http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.4/

LGTM.

-Chris.

[teststabilization] RFR 8229348: java/net/DatagramSocket/UnreferencedDatagramSockets.java fails intermittently

[Daniel Fuchs]

Hi,

Please find below a trivial fix for:

8229348: java/net/DatagramSocket/UnreferencedDatagramSockets.java
 fails intermittently
https://bugs.openjdk.java.net/browse/JDK-8229348

webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8229348/webrev.00/

This test has been observed failing intermittently in our CI.
The test failed in timeout - and there's no message saying
that the expected reply has been received or that any file
descriptor has been freed.
I suspect the test was blocked in receive() in its main() method
due to port reuse issues.

The usual fix for that is to avoid binding to the wildcard.

best regards,

-- daniel

Re: RFR [8203036]: com.sun.net.httpserver.HttpExchange should implement AutoCloseable

[Daniel Fuchs]

Hi,

This looks good to me Patrick.

best regards,

-- daniel

On 09/08/2019 12:09, Patrick Concannon wrote:

Hi,


Would it be possible to have my fix, and corresponding CSR, for 
JDK-8203036 reviewed?


This fix addresses the retrofitting of HttpExchange to implement 
AutoCloseable so that it can be used with try-with-resources statements.


The CSR for the fix can be found here: 
https://bugs.openjdk.java.net/browse/JDK-8229235
Further information on this bug can be found here: 
https://bugs.openjdk.java.net/browse/JDK-8203036


Webrev for fix: 
http://cr.openjdk.java.net/~michaelm/pconcannon/8229235/webrev.1/



Kind regards,

Patrick

Re: [teststabilization] RFR 8229348: java/net/DatagramSocket/UnreferencedDatagramSockets.java fails intermittently

[Chris Hegarty]

> On 9 Aug 2019, at 16:36, Daniel Fuchs  wrote:
> 
> Hi,
> 
> Please find below a trivial fix for:
> 
> 8229348: java/net/DatagramSocket/UnreferencedDatagramSockets.java
> fails intermittently
> https://bugs.openjdk.java.net/browse/JDK-8229348
> 
> webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8229348/webrev.00/

Looks ok.

-Chris.