8218559: Reimplement the Legacy Socket API
Alan Bateman
Alan.Bateman at oracle.com
Wed May 8 17:18:21 UTC 2019
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.
More information about the net-dev
mailing list