RFR: 8278326: Socket close is not thread safe and other cleanup

[Alan Bateman]

On Mon, 9 Jan 2023 10:35:18 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> java.net.Socket is not specified to be thread safe but it is required to support async close. If you create an unbound Socket and close it at around the time that another thread is binding, connecting, or anything else that creates the underlying socket then it can leak. The simplest thing to do is to synchronize all methods but the underlying SocketImpl implementation is thread safe, and all we really need is for Socket (and ServerSocket) to synchronize the creation of the underlying socket (SocketImpl.create) with close. As part of this change I've replaced the 6 flags with a bit mask. A new test is added to the Socket/asyncClose directory to test closing concurrently with another operation, the test will detect if the closed Socket is connected to a SocketImpl with an open socket.
>> 
>> Related is that ServerSocket.implAccept can be overridden to provide the Socket to accept. Its behavior is unspecified when called with a Socket that isn't newly created/unbound and there are number of silly scenarios that can arise. I've changed implAccept to coordinate with close so that accept doesn't return a closed Socket that is connected to an underlying socket. A new test is added to exercise these scenarios.
>> 
>> There are a couple of random cleanup/formatting nits in this patch.
>
> src/java.base/share/classes/java/net/Socket.java line 636:
> 
>> 634:             if (previous != null) {
>> 635:                 in = null;
>> 636:                 out = null;
> 
> Hello Alan, I haven't fully grasped this part of the change yet. I see that we now null out these input and outputstreams of the `Socket` instance. If an inputstream was handed out by this `Socket` instance (through a call to `Socket.getInputStream()`) and if the application code calls `InputStream.close()` after this `setConnectedImpl` has been called, then that call to `close()` can lead to closing this newly set `SocketImpl`. Is that OK? Or should that close on the "old" InputStream end up being a no-op? Like I noted, I still haven't fully wrapped my head around this `setConnectedImpl` so maybe this combination of calls is not  possible/practical?

ServerSocket.implAccept is intended to be called with a newly created, and unbound, Socket. We looked at this topic in JDK 13 (see JDK-8225431 and related issues) and decided the behavior is unspecified for silly/broken cases where it is called with a Socket in any other state. It should probably have been specified to throw ISE when introduced in JDK 1.1 but we decided there wasn't enough motive to try to fix the spec for that. This PR touches this area again because it interacts with Socket.close.

So the question is what is the JDK behavior when calling implAccept with a connected Socket. A Socket can't have two connections at the same time. The JDK behavior is to attach the SocketImpl for the newly accepted connection to the Socket. This breaks the association from the Socket to the previous SocketImpl. This PR doesn't change that. It does re-visit the question as to what to do with the now orphaned SocketImpl. Should it be discarded, in which case it may be a resource leak, or should it be closed?  The former seems preferable, in which case the read/write methods of streams handed out before calling implAccept will throw IOException. Your question is what happens if the input or output stream close method is called? The spec is that the close method closes the Socket so I've left this as is, it's not changed in this PR. The only real change here is that calling getInputStream/getOutputStream after accepting the new connection will return streams to the newly accepted connect
 ion. That seems the most sensible thing to do for this crazy case.

-------------

PR: https://git.openjdk.org/jdk/pull/11863