On Sun, 5 Dec 2021 16:44:05 GMT, Alan Bateman <al...@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
