RFR[8240533]: 'Inconsistent Exceptions are thrown by DatagramSocket and DatagramChannel when sending a DatagramPacket to port 0.'

[Patrick Concannon]

Hi,

Could someone please review my fix for JDK-8240533 'Inconsistent 
Exceptions are thrown by DatagramSocket and DatagramChannel when sending 
a DatagramPacket to port 0.' ?


Currently, DatagramSocket throws an IOException when sending to port 0, 
and doesn't throw when connecting. DatagramChannel currently throws a 
BindException in both cases.
The fix adds checks for port == 0 to the send and connect methods in 
DatagramSocket and DatagramChannelImpl. This is to ensure that 
consistent Exceptions i.e. SocketExceptions are thrown when trying to 
connect or send to port 0.



bug: https://bugs.openjdk.java.net/browse/JDK-8240533
webrev: http://cr.openjdk.java.net/~pconcannon/8240533/webrevs/webrev.00/


Kind regards,

Patrick

Re: Java 14 - Change in InetSocketAddress.toString() behaviour seems to be causing issues

[Jaikiran Pai]

Hello Alan,

On 30/03/20 1:31 pm, Alan Bateman wrote:
> On 28/03/2020 06:43, Jaikiran Pai wrote:
>>
>> There's an issue raised in Quarkus repo[1], where Apache Kafka
>> (embedded) no longer starts on Java 14. From what I can see the root
>> cause is this[2].
>>
>> JDK-8225499[3] changed the implementation (and even the javadoc) of
>> the InetSocketAddress.toString()
>>
> For the Quarkus/Kafka issue, do you know if it really needs to parse
> the String representation? Just curious why it doesn't use getHostString.

I don't have much knowledge about that library and unfortunately, the
git history on that file where this parsing is happening is very limited
and doesn't show the background about this code.

Either way, looking at the comment in that code which says:

// According to the Java 6 documentation, if the hostname is
    // unresolved, then the string before the colon is the hostname.


it appears that it was added way back during Java 6 days, before the
InetSocketAddress.getHostString() API was introduced in Java 7.

I checked their release page[1] and the 3.4.x series (the one which has
this issue) was released just almost a year back. So I think it might
still be used by many. I'll anyway bring this up in their issue tracker
to get their inputs.

>
> As I think you've found, the spec/implementation change in Java SE 14
> has two parts.
>
> The change to surround an IPv6 literal address (and maybe the scope
> ID) in square brackets. Anything that parses a string that encodes an
> IP address and port, say the host component in a URI string, will have
> seen this already.
>
> The change to concatenate "/" to unresolved addresses. I
> think it's important to observe that an InetSocketAddress can be
> created with nonsensical input so it's easy to confuse code that
> parses the string representation, e.g. new
> InetSocketAddress("java.com/1.2.3.4", 80) creates an InetSocketAddress
> that is unresolved but its string representation (when run on JDK 13
> or older) would suggest otherwise. 

Understood. Thank you!

[1] https://zookeeper.apache.org/releases.html

-Jaikiran

Re: RFR[8240533]: 'Inconsistent Exceptions are thrown by DatagramSocket and DatagramChannel when sending a DatagramPacket to port 0.'

[Lance Andersen]

Hi Patrick,

I think this looks OK.

I might suggest using assertThrows vs expectThrows.  While they are the same,  
assertThrows seems more natural and consistent with the other testNG assert 
methods.

HTH

Best
Lance

> On Mar 31, 2020, at 6:15 AM, Patrick Concannon  
> wrote:
> 
> Hi,
> 
> Could someone please review my fix for JDK-8240533 'Inconsistent Exceptions 
> are thrown by DatagramSocket and DatagramChannel when sending a 
> DatagramPacket to port 0.' ?
> 
> Currently, DatagramSocket throws an IOException when sending to port 0, and 
> doesn't throw when connecting. DatagramChannel currently throws a 
> BindException in both cases.
> The fix adds checks for port == 0 to the send and connect methods in 
> DatagramSocket and DatagramChannelImpl. This is to ensure that consistent 
> Exceptions i.e. SocketExceptions are thrown when trying to connect or send to 
> port 0.
> 
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8240533
> webrev: http://cr.openjdk.java.net/~pconcannon/8240533/webrevs/webrev.00/
> 
> 
> Kind regards,
> 
> Patrick

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: Java 14 - Change in InetSocketAddress.toString() behaviour seems to be causing issues

[Jaikiran Pai]

Hello Chris,

On 30/03/20 1:49 pm, Chris Hegarty wrote:
> Hi Jaikiran,
>
> Thank you for posting this message to net-dev.
>
> TL;DR seems like the specific issue raised against Quarkus has been
> resolved ( by upgrading to a more recent version of ZooKeeper ).

That's correct.

>
>> So this looks like an intentional(?)
>>
> Correct. This is an intentional change, to improve the string
> representation. See the following CSR
> https://bugs.openjdk.java.net/browse/JDK-8232002 
>>
>> breakage in that toString() API semantics?
>>
> FTR - I take exception with the term “breakage” - The change is
> intentional, consideration given to the compatibility impact, and a
> release note published. InetSocketAddress::getHostString was added in
> Java 7 to retrieve the host, or address, string representation.

Sorry about that term, I didn't mean to say that not enough thought was
spent on this change. I merely meant it to mean that the API behaviour
has changed.

>
>> I do see that it did make it to the release notes of this version,
>> but given how conservative Java has been in terms of breaking
>> changes, is this change too quick in introducing such a breaking change?
>>
> No. As above, code should really be using getHostString, which has
> been available since July 2011. I do not consider this too quick.

I agree about the getHostString() API being available since Java 7 days.
What I meant about this possibly being a bit too quick of change is
that, the toString() API of this class, even till Java 13, clearly
states that the part before the colon, of an unresolved address is a
host name. It doesn't however recommend using the getHostString(). So
there could be code which might still be using this API for the
semantics noted in Java 13.

Having said that, I would like to clarify that I don't have anything
against this change. I like that the unresolved address is now more
"visible".

>> By the way, keeping aside the breaking nature of this change, the
>> current javadoc, I think doesn't reflect what the current
>> implementation returns for unresolved addresses. The javadoc states:
>>
>> "If the address is unresolved, || is displayed in place
>> of the address literal."
>>
>> Note, the "in place of the address literal" part. However, in the
>> current implementation[6], it returns something like -
>> "localhost/:2182". So it doesn't just display the
>> "" literal in the address part but suffixes it to the
>> address part. Should that be clarified?
>>
> The InetSocketAddress string representation defers, partly, to
> InetAddress::toString, which specifies the “hostname / literal IP
> address” form. It is this address literal that is replaced with
> “”. If this is not clear from this spec, then it can be
> clarified.
>
Now that you explained it to me, I see what you mean and what that
javadoc means. I hadn't paid attention to the "This String is
constructed by calling toString() on the InetAddress and concatenating
the port number (with a colon)". Looking at the javadoc on
InetAddress.toString(), it does state the returned string to be of the
form "hostname/literal IP address" (example:
google.com/172.217.160.142). So when it says that " is
displayed in place of the address literal", it actually is talking about
the part that follows the "/" character in the output returned by the
InetAddress.toString().

I'll be honest - I didn't even know that up until this release, the
output of this API was something like google.com/172.217.160.142:80. I
had it in my mind that the output was google.com:80. That's why when the
new javadoc said "in place of the address literal", I thought it was
talking about replacing "google.com" with ""

I don't know if it's just me who couldn't fully understand it until this
was explained. For me, a {@link InetSocket.toString()} in the javadoc
and a couple of example representations of what the output of toString()
would look like would have made it easier. But I do understand that the
javadoc may not be the right place for such level of details.

> Can we check if the most recent ZooKeeper is still calling toString,
> using getHostString, or doing something else?
>
My quick look shows that in the newer version of that library there is
no such call (at least not in the context of using the return value to
get hold of the host string). However, I don't have enough experience
with that library. So I'll raise this in their issue tracker and point
them to this thread and see what they say.

-Jaikiran

Re: RFR[8240533]: 'Inconsistent Exceptions are thrown by DatagramSocket and DatagramChannel when sending a DatagramPacket to port 0.'

[Alan Bateman]

On 31/03/2020 11:15, Patrick Concannon wrote:

Hi,

Could someone please review my fix for JDK-8240533 'Inconsistent 
Exceptions are thrown by DatagramSocket and DatagramChannel when 
sending a DatagramPacket to port 0.' ?


Currently, DatagramSocket throws an IOException when sending to port 
0, and doesn't throw when connecting. DatagramChannel currently throws 
a BindException in both cases.
The fix adds checks for port == 0 to the send and connect methods in 
DatagramSocket and DatagramChannelImpl. This is to ensure that 
consistent Exceptions i.e. SocketExceptions are thrown when trying to 
connect or send to port 0.



bug: https://bugs.openjdk.java.net/browse/JDK-8240533
webrev: http://cr.openjdk.java.net/~pconcannon/8240533/webrevs/webrev.00/
Attempting to send or connect to port 0 is an error and the long 
standing behavior is to throw an IOException or SocketException so I 
think this looks good.


-Alan.

Re: RFR[8240533]: 'Inconsistent Exceptions are thrown by DatagramSocket and DatagramChannel when sending a DatagramPacket to port 0.'

[Daniel Fuchs]

This looks good to me Patrick.

I'm happy to see that all implementations now have a consistent
and predictable behavior.

Also - as Alan noted - this is not really a behavioral change
as some subclass of IOException (or IOException itself) was
already thrown before.

+1

best regards,

-- daniel

On 31/03/2020 11:15, Patrick Concannon wrote:

Hi,

Could someone please review my fix for JDK-8240533 'Inconsistent 
Exceptions are thrown by DatagramSocket and DatagramChannel when sending 
a DatagramPacket to port 0.' ?


Currently, DatagramSocket throws an IOException when sending to port 0, 
and doesn't throw when connecting. DatagramChannel currently throws a 
BindException in both cases.
The fix adds checks for port == 0 to the send and connect methods in 
DatagramSocket and DatagramChannelImpl. This is to ensure that 
consistent Exceptions i.e. SocketExceptions are thrown when trying to 
connect or send to port 0.



bug: https://bugs.openjdk.java.net/browse/JDK-8240533
webrev: http://cr.openjdk.java.net/~pconcannon/8240533/webrevs/webrev.00/


Kind regards,

Patrick

Re: RFR[8241072]: 'Reimplement Legacy DatagramSocket API'

[Alan Bateman]

On 30/03/2020 19:27, Patrick Concannon wrote:

Hi Alan,

Thanks for your feedback.

I've incorporated your comments into the revised webrev below.

http://cr.openjdk.java.net/~pconcannon/8241072/webrevs/webrev.01/


With regards to the UnreferencedXXX tests, I can take a look at these 
separately.

I skimmed through webrev.01 and just have a few comments:

NetMulticastSocket.checkOldImpl has been updated to check if the impl is 
null, I assume that is not needed as it is now checked in the constructor.


DatagramSocket.delegate() - I assume the exception message should be 
"Should not get here". An alternative here would be to just assert that 
delegate != null. Either is okay.


DatagramSocket L139-142. This comment/Note seems to be "notes to self" 
for possible future changes and maybe it should be removed to avoid 
confusing readers.


DatagramSocket.createDelegate L1144 can be simplified to "if 
(!initialized && delegate != null)".


DatagramSocketAdaptor uses 4-space indent rather than 8 so probably best 
to keep that style consistent if you can.


The webrev still has SendBufCheck. That is a test for another issue so 
I'll ignore this for now.


The rest looks good and a separate issue to replace the UnreferencedXXX 
tests is okay with me.


-Alan.

Re: RFR[8240533]: 'Inconsistent Exceptions are thrown by DatagramSocket and DatagramChannel when sending a DatagramPacket to port 0.'

[Chris Hegarty]

Patrick,

> On 31 Mar 2020, at 15:08, Daniel Fuchs  wrote:
> 
>> ..
>> bug: https://bugs.openjdk.java.net/browse/JDK-8240533
>> webrev: http://cr.openjdk.java.net/~pconcannon/8240533/webrevs/webrev.00/

Look good Patrick.

The check is deliberately performed after the security manager checks, right? 
If so, it is worth asserting this in a test.

-Chris.

Re: RFR[8241072]: 'Reimplement Legacy DatagramSocket API'

[Chris Hegarty]

> On 30 Mar 2020, at 19:27, Patrick Concannon  
> wrote:
> ...
> 
> http://cr.openjdk.java.net/~pconcannon/8241072/webrevs/webrev.01/

This looks very good, a testament to all the prior cleanup work that has 
already been done.

Just a few comments:

- DatagramSocket::getChannel now stands out a little, since it is one of the 
few instance methods has still has a non-delegating body. It could optionally 
delegate, or if not, then NetMulticastSocket doesn’t necessarily need to 
override it.

- In DatagramSocket::createDelegate, "enable broadcast if possible” - Possibly 
due to refactoring, but I cannot reconcile this with the old implementation.

  - The createDelegate's @return comment is a bit hard to parse; suggest: 
"@return {@code null} if {@code bindaddr == NO_DELEGATE}, OTHERWISE returns a 
delegate for the requested {@code type}"

- The set of socket options is now per datagram/multicast socket instance, 
rather than on the class of the socket. I don’t think that this is an issue, 
just an observation and a confirmation that it is deliberate.

-Chris.

Re: RFR[8241072]: 'Reimplement Legacy DatagramSocket API'

[Alan Bateman]

On 31/03/2020 18:27, Chris Hegarty wrote:

:
- In DatagramSocket::createDelegate, "enable broadcast if possible” - Possibly 
due to refactoring, but I cannot reconcile this with the old implementation.
DatagramSocket is specified to make a best attempt to enable this option 
so I think it's in the right place. At a clean-up, the attempt in 
PDSI.datagramSocketCreate could be removed.




:

- The set of socket options is now per datagram/multicast socket instance, 
rather than on the class of the socket. I don’t think that this is an issue, 
just an observation and a confirmation that it is deliberate.
The set of socket options supported by a custom DSI will likely be 
different to the default impl so it needs to be an instance field. I 
suspect this is a long standing bug.


-Alan