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.
