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

[Jaikiran Pai]

On Mon, 9 Jan 2023 12:15:39 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> 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 conne
 ction. That seems the most sensible thing to do for this crazy case.

Thank you for that detailed explanation, Alan. That helped.

> 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.

I'm guessing that is a typo and you actually mean "The latter seems preferable ..."? The implementation in this PR  does the latter and closes the previous orphaned SocketImpl and I think that's the right thing to do.

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

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