8218559: Reimplement the Legacy Socket API

[Chris Hegarty]

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.