RFR: 8223553: Fix code constructs that do not compile with the Eclipse Java Compiler
Hi, please review a small change that I'd like to see in OpenJDK to get rid of compilation errors in the Eclipse IDE. It seems the root cause for the compilation errors is that javac would sometimes widen capture variables and is hence able to compile the places that I touch here. The EC4J compiler claims that it's following the spec more strictly and bails out at these places. I had posted about this on compiler-dev but got no reaction [0]. So, as this seems to be some field of open work for the compiler/spec folks, I'd like to get EC4J quiesced by introducing some slight modifications to the original places which makes the code appeal a bit less elegant and fluent but will get rid of the compile errors. Please review: Bug: https://bugs.openjdk.java.net/browse/JDK-8223553 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8223553.0/ The modification to src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java belongs to JSR-166, so I don't know if it needs some special handling. Thanks & Best regards Christoph [0] https://mail.openjdk.java.net/pipermail/compiler-dev/2019-March/013054.html
Re: [ipv6] RFR: 8223532: Don't try creating IPv4 sockets in NetworkInterface.c if IPv4 is not supported
Hi Arthur, The idea looks reasonable to me - I believe the changes in NetworkInterface.c should be OK. However - I don't think the changes in net_util.c::DEF_JNI_OnLoad are acceptable. I'll question whether this is the right place to bail out. I agree it would be nice to have some diagnostic that tells us that the native library is non-functioning because both IPv4 & IPv6 where not available, but I'd rather have this check at the place where the code actually tries to e.g. open an IP/TCP socket, and where throwing a regular exception with a better detailed message is possible, rather than having the VM fail to start with some cryptic UnsatisfiedLinkError. best regards, -- daniel On 08/05/2019 06:52, Arthur Eubanks wrote: Bug: https://bugs.openjdk.java.net/browse/JDK-8223532 Webrev: http://cr.openjdk.java.net/~aeubanks/8223532/webrev.00/ There are a couple of functions in src/java.base/unix/native/libnet/NetworkInterface.c which try to create IPv4 sockets, then check if errno is EPROTONOSUPPORT. If errno is EPROTONOSUPPORT, then IPv6 is tried, else it immediately bails. We have a kernel that returns EAFNOSUPPORT when IPv4 is disabled and there is an attempt to create an AF_INET socket. Rather than adding a check for EAFNOSUPPORT, which seems too specific, just don't bother trying to create an IPv4 socket if IPv4 is not available. This uses ipv4_available(), introduced in http://hg.openjdk.java.net/jdk/jdk/rev/22323f20401b. Since previously the code assumed that we have at least IPv4, now when initializing ipv4/6_available(), make sure that at least one of them is available, or else bail. One problem I have with this is that the error message when neither is present is not descriptive: Error occurred during initialization of boot layer java.lang.UnsatisfiedLinkError: unsupported JNI version 0x required by path/to/jdk/build/jdk/lib/libnet.so
Re: [ipv6] Regarding 8220673: Add test library support for determining platform IP support
Arthur,
On 07/05/2019 19:35, Arthur Eubanks wrote:
...
With help from Chris's references ([1]), I added a test with a policy
file that grants IPSupport permission to listen/resolve localhost:0, and
read the java.net.preferIPv4Stack system property. Without either
permission, the test fails as expected.
One thing I discovered is that to make the policy file work properly
with the jtreg directory structure, I needed to add "@build
jdk.test.lib.net.IPSupport" to the test. So any test that wants to use
IPSupport and security managers needs to have that extra line and make
sure that its policy file contains all the permissions as in the new
test policy I added.
That is ok.
Without "@build jdk.test.lib.net.IPSupport", the policy file would need
grant codeBase "file:${test.classes}/-"
With "@build jdk.test.lib.net.IPSupport", the policy file needs grant
codeBase "file:${test.classes}/../../../../test/lib/-"
Right, the difference comes from how jtreg builds its dependencies. The
former is an example of when a library is used conveniently with just
the @library tag, the library is automatically compiled into the
specific test's classes directory. The latter is when the library is
explicitly compiled with an @build, and gets put into a shared library
output directory, so that other tests ( that also specify an @build with
that library name ) can use the pre-built classes.
This matches [1], and seems more restrictive (only applies to test
library code) and better.
New webrev: http://cr.openjdk.java.net/~aeubanks/8220673/webrev.03/
I am happy with this latest version. Reviewed.
Trivially, maybe rename B8220673.java to something more descriptive
( without the bug no. ). Maybe MinimumPermissions? or whatever,
no need for a new webrev.
-Chris.
P.S. adding nio-dev since there are a few tests in that area
being updated.
[1]:
https://hg.openjdk.java.net/jdk/jdk/file/1dc9bf9d016b/test/jdk/java/net/httpclient/AsFileDownloadTest.policy#l24
Re: [ipv6] Regarding 8220673: Add test library support for determining platform IP support
Hi Arthur,
On 07/05/2019 19:35, Arthur Eubanks wrote:
When you said "double checking" I thought it was some terminology
related to security managers (e.g. "double checked locking") :P
Oh - sorry I was not clear enough.
I looked through all the tests I modified for any references to security
managers or policy files (grep "security" and "policy"). There is
one, test/jdk/java/net/InetAddress/GetLocalHostWithSM.java, but the call
to IPSupport is before the test sets the SecurityManager.
So the existing tests should not be affected by this.
Thanks :-) That's what I wanted to hear.
With help from Chris's references ([1]), I added a test with a policy
file that grants IPSupport permission to listen/resolve localhost:0, and
read the java.net.preferIPv4Stack system property. Without either
permission, the test fails as expected.
One thing I discovered is that to make the policy file work properly
with the jtreg directory structure, I needed to add "@build
jdk.test.lib.net.IPSupport" to the test. So any test that wants to use
IPSupport and security managers needs to have that extra line and make
sure that its policy file contains all the permissions as in the new
test policy I added.
Without "@build jdk.test.lib.net.IPSupport", the policy file would need
grant codeBase "file:${test.classes}/-"
With "@build jdk.test.lib.net.IPSupport", the policy file needs grant
codeBase "file:${test.classes}/../../../../test/lib/-"
This matches [1], and seems more restrictive (only applies to test
library code) and better.
Ah! yes - there's this old (odd?) behavior about where jtreg
generates the library test classes. That has bitten us in the
past - mostly because if you start using using @build then
the test must explicitly list all classes it intends to use.
Otherwise, some test might pass 'by chance' because some
other test has run before and caused jtreg to compile the
classes it needs. The classic symptom would be that the test passes
when you run :jdk_net but fail when you run it in isolation
(when it's the only test that your jtreg command line compiles).
So if you add @build jdk.test.lib.net.IPSupport then please
make sure to add all the library classes that the test might
be using on the @build command line (IIRC wildcards are also
accepted). Just mentioning it for the future as I see these
changes shouldn't have this issue.
New webrev: http://cr.openjdk.java.net/~aeubanks/8220673/webrev.03/
http://cr.openjdk.java.net/~aeubanks/8220673/webrev.03/test/jdk/java/net/MulticastSocket/JoinLeave.java.frames.html
has two `@library /test/lib` lines
The rest looks very good to me!
Thanks for taking that on, and thanks for adding the new test!
best regards,
-- daniel
[1]:
https://hg.openjdk.java.net/jdk/jdk/file/1dc9bf9d016b/test/jdk/java/net/httpclient/AsFileDownloadTest.policy#l24
Re: [ipv6] RFR: 8223532: Don't try creating IPv4 sockets in NetworkInterface.c if IPv4 is not supported
Arthur, Daniel, On 08/05/2019 11:45, Daniel Fuchs wrote: Hi Arthur, The idea looks reasonable to me - I believe the changes in NetworkInterface.c should be OK. I agree. Now that I look at this again, I think it may also be reasonable to consider EAFNOSUPPORT in a similar way to that of EPROTONOSUPPORT. To me it is really a matter of interpretation by the Kernel developer which gets thrown and at which point ( protocol / family /type compiled out of the Kernel or disabled by the System Administrator ). Anyway, what Arthur is proposing for NetworkInterface is fine. However - I don't think the changes in net_util.c::DEF_JNI_OnLoad are acceptable. Agree. I'll question whether this is the right place to bail out. I agree it would be nice to have some diagnostic that tells us that the native library is non-functioning because both IPv4 & IPv6 where not available, but I'd rather have this check at the place where the code actually tries to e.g. open an IP/TCP socket, and where throwing a regular exception with a better detailed message is possible, rather than having the VM fail to start with some cryptic UnsatisfiedLinkError. The cryptic error message ( generated by returning a JNI version that is not supported by the VM ) indicates that this is not the right place. ... On 08/05/2019 06:52, Arthur Eubanks wrote: ... Since previously the code assumed that we have at least IPv4, now when initializing ipv4/6_available(), make sure that at least one of them is available, or else bail. One problem I have with this is that the error message when neither is present is not descriptive: Error occurred during initialization of boot layer java.lang.UnsatisfiedLinkError: unsupported JNI version 0x required by path/to/jdk/build/jdk/lib/libnet.so While the vast majority of libnet.so is devoted to socket related implementation, not all is. There are a small number of low-level pieces of functionality that can be used with support for either IPv4 or IPv6 being present. The NIO implementation also uses some shared common functionality from libnet.so. It seems overly restricting to disallow libnet.so from loading if neither IPv4 or IPv6 are present. I'm just not sure that this is needed at all. -Chris.
Re: [ipv6] RFR: 8223532: Don't try creating IPv4 sockets in NetworkInterface.c if IPv4 is not supported
On 08/05/2019 12:51, Chris Hegarty wrote: While the vast majority of libnet.so is devoted to socket related implementation, not all is. There are a small number of low-level pieces of functionality that can be used with support for either IPv4 or IPv6 being present. The NIO implementation also uses some shared common functionality from libnet.so. It seems overly restricting to disallow libnet.so from loading if neither IPv4 or IPv6 are present. I agree as an innocent reference to a type in java.net might resulting in libnet being loaded as a side effect. We also have the issue that the file system implementation is in libnio so libnet will be loaded there too (although shouldn't trigger its JNI OnLoad to be run). -Alan
Re: 8218559: Reimplement the Legacy Socket API
Alan,
On 01/05/2019 14:10, Alan Bateman wrote:
JEP 353 [1] is now a candidate and I would like to get the CSR [2]
finalized and the changes reviewed so that it can be targeted.
The webrev with the changes is here:
http://cr.openjdk.java.net/~alanb/8221481/1/webrev/index.html
This is a nice. The code is well structured and much more readable than
the plain implementation.
Some specific comments:
NioSocketImpl.java
If configureBlocking returned a boolean, then an assert could be
written to ensure that it operates as expected. For example, both
connect and accept require it to actually switch the block mode for
timed operations when called ( it cannot be a no-op ).
RuntimeException is being used to transport IOException between
FileDescriptorCloser::run ( which cannot throw a checked exception )
and tryClose. I believe that tryClose should catch and unwrap this
carrier RuntimeException.
When tryClose is invoked in end[Read|Write|Connect|Accept], I believe
that IOExceptions should be suppressed, at least when `completed` is
true.
typos "an" -> "a"
408 * @throws SocketException if the socket is closed or *an* socket
I/O error occurs
429 * @throws SocketException if the socket is closed or *an* socket
I/O error occurs
882 // shutdown output when linger interval not set to 0
883 try {
884 var SO_LINGER = StandardSocketOptions.SO_LINGER;
885 if ((int) Net.getSocketOption(fd, SO_LINGER) != 0) {
886 Net.shutdown(fd, Net.SHUT_WR);
887 }
888 } catch (IOException ignore) { }
Is this a bug fix, or something that you ran across? I'm not familiar
with this.
SocketImpl.java
"45 * @implNote Client and server sockets created with the {@code
Socket} and
46 * {@code SocketServer} public constructors create a
system/platform default
47 * {@code SocketImpl}."
The public no-args j.n.Socket constructor has the following slightly
different wording: "Creates an unconnected socket, with the
system-default type of SocketImpl." - these should probably be
consistent.
Why the use of "Java virtual machine" and "command line", since it is
just a system property. I assume that this must come from the read-once
nature of its impact? If so, are there other example of such and where?
49 * newer implementation. The JDK continues to ship with the older
implementation
50 * to allow code to run that depends on unspecified behavior that
differs between
51 * the old and new implementations.
If the intention of this property is as a temporarily aid for folk,
that have code that depend upon the unspecified behavior, to run
successfully until that code can be amended to remove the dependency on
the particular unspecified behavior, then can that please be made
clear.
The property is effectively terminally deprecated at birth, right? If
so, can a note indicating such be added.
Timeouts.java
The test uses `assertTrue(false);` in a few places. TestNG has
`org.testng.Assert.fail(String)` for this.
Only time will tell if these tests that check duration will be stable
on all platforms and configurations.
UdpSocket.java
The new test scenario uses a hardcoded port number. Could this be a
problem?
121 while (ref.get() != null) {
122 System.gc();
123 Thread.sleep(100);
124 }
Is this loop guaranteed to exist at some point?
80 assertTrue(Arrays.equals(array1, 0, n, array2, 0, n), "Unexpected
contents");
Can use `assertEquals(byte[] actual, byte[] expected, String message)`,
which will print the arrays if unequal.
NewSocketMethods.java
Is the change in this class still needed, or left over from a previous
iteration?
test/jdk/java/net/SocketImpl/BadUsages.java
At one point I added test/jdk/java/net/SocketImpl/BadSocketImpls.java
to the sandbox branch. Have the test scenarios been subsumed into
BadUsages? Or where have they gone?
-Chris.
Re: [ipv6] Regarding 8220673: Add test library support for determining platform IP support
On 08/05/2019 17:02, Arthur Eubanks wrote: http://cr.openjdk.java.net/~aeubanks/8220673/webrev.04/ LGTM. best regards, -- daniel
Re: 8218559: Reimplement the Legacy Socket API
On 08/05/2019 16:25, Chris Hegarty wrote:
This is a nice. The code is well structured and much more readable than
the plain implementation.
Thanks for doing through the implementation.
:
NioSocketImpl.java
If configureBlocking returned a boolean, then an assert could be
written to ensure that it operates as expected. For example, both
connect and accept require it to actually switch the block mode for
timed operations when called ( it cannot be a no-op ).
It's just connect that restores the blocking mode after a connection is
established with a timeout. IOException is thrown if it fails. I assume
you are wondering if there is a code path that calls
configureNonBlockingForever that locks the socket in non-blocking mode.
I guess we could add asserts but that would complicate down stream work.
So I guess I'm inclined to leave it as is, and add any comments to help
if there isn't anything obvious on how it works.
RuntimeException is being used to transport IOException between
FileDescriptorCloser::run ( which cannot throw a checked exception )
and tryClose. I believe that tryClose should catch and unwrap this
carrier RuntimeException.
Fair point as there is an assumption it cannot fail. There is actually
is a long standing issue here in that
FileDispatcherImpl.closeFileDescriptor should not throw an exception if
close fails with EINTR (as close is not restartable). If it fails with
anything else then it suggests a serious bug somewhere.
:
typos "an" -> "a"
408 * @throws SocketException if the socket is closed or *an*
socket I/O error occurs
429 * @throws SocketException if the socket is closed or *an*
socket I/O error occurs
Thanks.
882 // shutdown output when linger interval not set to 0
883 try {
884 var SO_LINGER = StandardSocketOptions.SO_LINGER;
885 if ((int) Net.getSocketOption(fd, SO_LINGER) != 0) {
886 Net.shutdown(fd, Net.SHUT_WR);
887 }
888 } catch (IOException ignore) { }
Is this a bug fix, or something that you ran across? I'm not familiar
with this.
This preserves existing behavior to do a graceful shutdown when there is
no linger interval. If you look at the NET_SocketClose implementation on
Windows or SocketChannelImpl.close then you'll see what I mean.
SocketImpl.java
"45 * @implNote Client and server sockets created with the {@code
Socket} and
46 * {@code SocketServer} public constructors create a
system/platform default
47 * {@code SocketImpl}."
The public no-args j.n.Socket constructor has the following slightly
different wording: "Creates an unconnected socket, with the
system-default type of SocketImpl." - these should probably be
consistent.
Okay, I can try to keep it consistent although at some point I think we
should do a pass over some of the JDK 1.0 era javadoc and re-visit the
places it talks about the default SocketImpl, esp. all the references to
"plain".
Why the use of "Java virtual machine" and "command line", since it is
just a system property. I assume that this must come from the read-once
nature of its impact? If so, are there other example of such and where?
It's not reliable to set the system property in a running VM so the note
is just setting the expectations that it needs to be set at startup,
nothing more.
49 * newer implementation. The JDK continues to ship with the older
implementation
50 * to allow code to run that depends on unspecified behavior that
differs between
51 * the old and new implementations.
If the intention of this property is as a temporarily aid for folk,
that have code that depend upon the unspecified behavior, to run
successfully until that code can be amended to remove the dependency on
the particular unspecified behavior, then can that please be made
clear.
The property is effectively terminally deprecated at birth, right? If
so, can a note indicating such be added.
The intention is that implNote will be removed when the property and old
implementation are removed. It's something that I was going to put in
the RN/docs but I guess we could extend the implNote on this point too.
Timeouts.java
The test uses `assertTrue(false);` in a few places. TestNG has
`org.testng.Assert.fail(String)` for this.
Okay.
Only time will tell if these tests that check duration will be stable
on all platforms and configurations.
Right, it's always tricky to test timeout behavior. In this case, I've
run the test 1000+ times on all platforms and have not seen any failures.
UdpSocket.java
The new test scenario uses a hardcoded port number. Could this be a
problem?
The host/port specified to the 3-arg constructor is the remote socket
address that it connects to, not the local address/port. So it's like
DatagramSocket.connect and any port will do because it's not
sending/receiving packets.
121 while (ref.get() != null) {
122
[teststabilization] RFR: 8223573: Replace wildcard address with loopback or local host in tests - part 4
Hi, Please find below another patch in the series for intermittently failing tests: 8223573: Replace wildcard address with loopback or local host in tests - part 4 https://bugs.openjdk.java.net/browse/JDK-8223573 patch: http://cr.openjdk.java.net/~dfuchs/webrev_8223573/webrev.00/ best regards, -- daniel
Re: [ipv6] Regarding 8220673: Add test library support for determining platform IP support
On 08/05/2019 12:02, Chris Hegarty wrote: : New webrev: http://cr.openjdk.java.net/~aeubanks/8220673/webrev.03/ : P.S. adding nio-dev since there are a few tests in that area being updated. Thanks for forwarding. IPSupports.skipIfConcurrentConfigurationIsInvalid is a bit strange and I think the first use of SkippedException in the jdk tests. I'm tempted to suggest this should be a key that tests should select on via the @requires tag but that may not be feasible because it depends on properties such as preferIPv4Stack. So I guess the approach is okay but we should rename the method to start with "skipTest" as it's not clear from the use-case what is being skipped. Also "ConcurrentConfigurationIsInvalid" is vague and I think it would be useful if the method description could emulate the conditions a bit better so that someone seeing a IPSupport.skipTestXXX knows what is means. -Alan
