8218559: Reimplement the Legacy Socket API

[Alan Bateman]

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                 System.gc();
>   123                 Thread.sleep(100);
>   124             }
>
>  Is this loop guaranteed to exist at some point?
We use this idiom in several tests. If someone where to run the tests 
with +DisableExplicitGC then it would fail but hundreds of other tests 
would also fail.

>
>  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.
Fair point although this is the existing test where it just throws when 
the size or contents differ.


>
> NewSocketMethods.java
>
>  Is the change in this class still needed, or left over from a previous
>  iteration?
This is an outlier on Solaris as that platform doesn't allow IPV6_TCLASS 
to be set when the socket is connected. There is a note in the JEP on 
this as it is masked by the old implementation. I see you've collected 
some useful info in JDK-8221487 on the issue so I think we can keep it 
separate.


>
> 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?
BadSocketImpls.java exercised the platform SocketImpl accepting a 
connection with a no-op SocketImpl.  It pre-dates the work in 
JDK-8220493 to prevent such combinations. We added 
SocketImplCombinations.java as part of JDK-8220493 to test all the 
combinations and scenarios that we could come up, I don't think there 
was anything in BadSocketImpls that we could bring over.

-Alan.