Re: [15] RFR 8241760 : Typos: empty lines in javadoc, inconsistent indents, etc. (net and nio)

[Ivan Gerasimov]

Thank you guy for your reviews!

Pushed.

On 3/30/20 5:27 AM, Daniel Fuchs wrote:

Looks good to me Ivan.

Thanks for this cleanup!


best regards,

-- daniel

On 30/03/2020 03:34, Ivan Gerasimov wrote:

Thank you Alan and Pavel!

My apologies for a wrong link in the initial request.

Here's the new webrev with the changes suggested by Pavel:

http://cr.openjdk.java.net/~igerasim/8241760/01/webrev/

With kind regards,

Ivan G.


--
With kind regards,
Ivan Gerasimov

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

[Patrick Concannon]

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.


-Patrick

On 23/03/2020 18:59, Alan Bateman wrote:

On 23/03/2020 16:37, Patrick Concannon wrote:
This is a request for review for the implementation changes for JEP 
373: 'Reimplement Legacy DatagramSocket API' [1]. A CSR has also been 
created [2].


The implementation of JEP 373 can be viewed on the JDK-8230211-branch 
in the sandbox [3].


A link to the webrev with the cumulated changes is available below [4].

The sources changes are limited to five files. The webrev also adds 
additional tests while a selection of existing tests have been 
updated to run with both the old and new implementation. Please refer 
to the JEP [1] for more information.

This is a good work and easy to review.

As you know, I would have preferred to see the creation of the 
delegate separated from the configuration (like we do with impls) but 
I think you and Daniel want to keep the version you have, okay. I'm 
also not too keen on another temporary solution in advance of any 
discussion on JDK-8237352 but I guess what you have is okay as it's 
benign.


Small comments:

DatagramSocket
- the comments at L117-120, L123-124, and L143-L147 need a tidy up, 
probably most of them can be replaced with simple one-liners.
- the use of type.cast at L119 and L1125 looks odd. It might be 
simpler to just replace L1156 with "return (T) delegate". That would 
allow you to assign delegate (no need for "adaptor" or oddly named 
"plainDelegate").

- I assume the new static final fields can be private too.
- usePlainDatagramSocketImpl is inconsistent with the equivalent in 
SocketImpl for the garbage value case. We should try to get those 
consistent at some point.

- createDelegate's method description has inconsistent formatting.
- L1145 should be "initialized = true".

MulticastSocket and NetMulticastSocket look okay. There are probably 
better names for "NetMulticastSocket" but it's not public so okay. 
Happy to see NetMulticastSocket.options is an instance field, seems 
like that bug was lurking for a long time.


DatagramSocketAdaptor - if we are keeping this change then I'd prefer 
if the comment was cleaned up to be consistent with the other support 
classes. A one-liner is okay. The "Temporary solution" comment can go 
away. Also would be good to use 4-space indentation to be consistent 
with the other support classes.


The changes to run the tests twice is good. The UnreferencedXXX tests 
are a bit messy and I'm concerned that the changes to these tests make 
them too hard to maintain. I think we need to find better ways to test 
that unreferenced sockets are closed so that these tests can be 
replaced with tests that easy to maintain.


SendBufCheck is puzzling to see in this webrev as it's not part of the 
JEP. An equivalent of JDK-8239355 that only tests the new 
implementation (not the old because it's not quite right) would be 
okay. Alternatively re-visit it as part of JDK-8240901 to test that 
large datagrams can be sent on the network.


I think that is all I have.

-Alan

Re: RFR JDK-8239595/JDK-8239594 : ssl context version is not respected/jdk.tls.client.protocols is not respected

[rahul . r . yadav]

The current fix does not affect the scenarios discussed earlier(that is 
a broader discussion,may be a different bug/enhancement).

The scenarios would be vaild even if the fix would not have been in place.

-Rahul

On 27/03/2020 17:50, Chris Hegarty wrote:
Thank you for these clarifications. We will now consider how these 
affect, if at all, the HTTP Client.


-Chris.

On 27 Mar 2020, at 17:47, Xuelei Fan > wrote:


On 3/27/2020 10:36 AM, Chris Hegarty wrote:

Thank you Xuelei, this very helpful.
Sorry, but I am going to ask just a few more clarifying questions to 
make sure that we’re on the same page.
On 27 Mar 2020, at 16:23, Xuelei Fan > wrote:


On 3/27/2020 5:52 AM, Chris Hegarty wrote:

Xuelei,
Before commenting further on the interaction of the HTTP Client 
with various contorted configurations, I would like to get a 
better understanding of the `jdk.tls.client.protocols` property.
Is there a specification or other documentation describing 
`jdk.tls.client.protocols` ?
See the jdk.tls.client.protocols line in table 'Table 8-3 System 
Properties and Customized Items" in JSSE Reference Guides:


"https://docs.oracle.com/en/java/javase/14/security/java-secure-socket-extension-jsse-reference-guide.html#GUID-A41282C3-19A3-400A-A40F-86F4DA22ABA9

For your quick reference, I copied the note here:

---
Customized Item:
Default handshaking protocols for TLS/DTLS clients.

Notes:
To enable specific SunJSSE protocols on the client, specify them in 
a comma-separated list within quotation marks; all other supported 
protocols are not enabled on the client
“supported” here means protocols that are supported by the provider, 
and may be used within a specific context. This translates, for the 
default SSLContext, to the API call 
getSupportedSSLParameters().getProtocols(), right?

Yes.

getSupportedSSLParameters().getProtocols() returns a superset of 
getDefaultSSLParameters().getProtocols(). Conversely, 
getDefaultSSLParameters().getProtocols() is a strict subset of 
getSupportedSSLParameters().getProtocols(), right?

Yes.

The `jdk.tls.client.protocols` property has no affect on 
getSupportedSSLParameters().getProtocols()  only 
getDefaultSSLParameters().getProtocols(), right?

Yes.

In which case, getDefaultSSLParameters().getProtocols() returns the 
value of  `jdk.tls.client.protocols`.

For example,

   If jdk.tls.client.protocols="TLSv1,TLSv1.1", then the default 
protocol settings on the client for TLSv1 and TLSv1.1 are enabled, 
while SSLv3, TLSv1.2, TLSv1.3, and SSLv2Hello are not enabled


   If jdk.tls.client.protocols="DTLSv1.2" , then the protocol 
setting on the client for DTLS1.2 is enabled, while DTLS1.0 is not 
enabled

---
Seems that the term “client” here is referring to client-initiated 
exchanges, rather than any specific technology.
The assumption, which is reasonable, is that “clients” will use the 
default context. Again, this is reasonable default out-of-the-box 
behavior.
The client refer to the client side SSLSocket or SSLEngine created 
with the default SSLContext.  or example:

   SSLContext sslContext = SSLContext.getInstance("TLS");
   SSLEngine sslEngine = sslContext.createSSLEngine();
   sslEngine.setUseClientMode(true);

The sslEngine object is a client that impacted by the property.

While if
   sslEngine.setUseClientMode(false);

then the object should not be impacted by the property.

Xuelei

It is my understanding that the property only affects the 
*default* protocol’s ( not the supported protocols ) of the 
*default* context. That is, the context returned by 
`SSLContext.getInstance("Default”)`,
It is correct that the property impact the default SSLContext only. 
 The default SSLContext instance could get from:

   SSLContext.getInstance("Default");
   SSLContext.getInstance("TLS");
   SSLContext.getInstance("DTLS”);

Thanks for this clarification.


and the protocol values returned by the following invocation on 
that context `getDefaultSSLParameters().getProtocols()`. Is this 
correct? If not, what does it do?

Yes.

Thanks,
-Chris.

Re: [15] RFR 8241760 : Typos: empty lines in javadoc, inconsistent indents, etc. (net and nio)

[Daniel Fuchs]

Looks good to me Ivan.

Thanks for this cleanup!


best regards,

-- daniel

On 30/03/2020 03:34, Ivan Gerasimov wrote:

Thank you Alan and Pavel!

My apologies for a wrong link in the initial request.

Here's the new webrev with the changes suggested by Pavel:

http://cr.openjdk.java.net/~igerasim/8241760/01/webrev/

With kind regards,

Ivan G.

Re: [15] RFR 8241760 : Typos: empty lines in javadoc, inconsistent indents, etc. (net and nio)

[Pavel Rappo]

Looks good. Thanks.

> On 30 Mar 2020, at 03:34, Ivan Gerasimov  wrote:
> 
> Thank you Alan and Pavel!
> 
> My apologies for a wrong link in the initial request.
> 
> Here's the new webrev with the changes suggested by Pavel:
> 
> http://cr.openjdk.java.net/~igerasim/8241760/01/webrev/
> 
> With kind regards,
> 
> Ivan G.
> 
> 
> On 3/29/20 2:43 PM, Pavel Rappo wrote:
>> Ivan,
>> 
>> 1. ByteBuffered has an awkwardly looking top-level doc comment 
>> markup/formatting.
>> The "payload" begins on the same line as the /** marker. Also, the  tag is
>> weirdly placed. Since you have fixed similar issues already (URLConnection),
>> you could probably do the same here. Your call.
>> 
>> 2. I know you probably wanted to confine this change to just the formatting,
>> but... maybe we could fix the "protocol handers" typo as an exception?
>> 
>> Other than that, the changes look good. Thanks for doing this!
>> 
>> -Pavel
>> 
>>> On 29 Mar 2020, at 05:44, Ivan Gerasimov  wrote:
>>> 
>>> Hello!
>>> 
>>> The fix follows up on JDK-8241727 [1].
>>> 
>>> This is a javadoc/comments only fix in the net and nio areas.
>>> 
>>> The changes are to remove redundant empty lines, correct indentation, or 
>>> otherwise restore harmony.
>>> 
>>> Would you please help review this rather technical fix?
>>> 
>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8241727
>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8241727/00/webrev/
>>> 
>>> Thank in advance!
>>> 
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8241727
>>> 
>>> -- 
>>> With kind regards,
>>> Ivan Gerasimov
>>> 
> -- 
> With kind regards,
> Ivan Gerasimov
> 

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

[Chris Hegarty]

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

> On 28 Mar 2020, at 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() which now reads[4]:
> 
> "... 
> 
> If the address is unresolved,  is displayed in place of the 
> address literal.
> 
> "
> 
> Previously, until as recent as Java 13[5], it used to be:
> 
> "...
> If the address is unresolved then the part before the colon will only contain 
> the hostname.
> 
> "
> 
> 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.

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

Can we check if the most recent ZooKeeper is still calling toString, using 
getHostString, or doing something else?

-Chris.

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

[Alan Bateman]

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. To 
my knowledge anyway, this is the first report of an issue with this 
spec/implementation change. It's a reminder that we need a lot more 
testing of the JDK early access builds to gauge the impact of fixes like 
this.


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. 
I'll have to defer to Julia, Daniel and Chris to see what other options 
were considered. I could imagine conditionally adding ""/"  
based on whether host string contains slash and/or colon characters. 
That might reduce the compatibility impact but at the cost of additional 
complexity and/or non-obvious failures in code that encounter 
nonsensical string on rare occasions.


-Alan