RFR: 8278270: ServerSocket is not thread safe

[Aleksey Shipilev]

On Sun, 5 Dec 2021 16:44:05 GMT, Alan Bateman <alanb at openjdk.org> wrote:

> There are several thread safety issues in java.net.ServerSocket, issues that go back to at least JDK 1.4.
> 
> The issue of most concern is async close of a ServerSocket that is initially created unbound and where close may be called at or around the time the underlying SocketImpl is created or the socket is bound.
> 
> The summary of the changes are:
> 
> 1. The "impl" field is changed to be final field.
> 2. The closeLock is renamed to stateLock and is required to change the (now volatile) created, bound or closed fields.
> 3. The needless synchronization has been removed from xxxSoTimeout and xxxReceiveBufferSize.
> 
> There are many redundant checks for isClosed() and other state that could be removed. Removing them would subtle change the exception thrown when there are two or more failure conditions. So they are left as is.

src/java.base/share/classes/java/net/ServerSocket.java line 90:

> 88:     private volatile boolean created;   // impl.create(boolean) called
> 89:     private volatile boolean bound;
> 90:     private volatile boolean closed;

I don't see why these two need to be `volatile`, but we can keep them for extra safety.

src/java.base/share/classes/java/net/ServerSocket.java line 712:

> 710:      */
> 711:     public void close() throws IOException {
> 712:         synchronized (stateLock) {

Maybe makes some sense to check `closed` outside the lock here? Looks like we don't need to re-acquire the lock for repeated `close()` calls.

src/java.base/share/classes/java/net/ServerSocket.java line 804:

> 802:      * @see #setSoTimeout(int)
> 803:      */
> 804:     public int getSoTimeout() throws IOException {

Is it safe to drop `synchronized` here? Any subclasses/implementations rely on mutual exclusion here?

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

PR: https://git.openjdk.java.net/jdk/pull/6712