Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8278326: Socket close is not thread safe and other cleanup #11863

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -124,8 +124,8 @@ protected void connect(SocketAddress endpoint, int timeout)
// close the original socket impl and release its descriptor
close();

// update the Sockets impl to the impl from the http Socket
SocketImpl si = httpSocket.impl;
// change Socket to use httpSocket's SocketImpl
SocketImpl si = httpSocket.impl();
socket.setImpl(si);

// best effort is made to try and reset options previously set
Expand Down
117 changes: 62 additions & 55 deletions src/java.base/share/classes/java/net/ServerSocket.java
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1995, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1995, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -78,18 +78,23 @@
* @since 1.0
*/
public class ServerSocket implements java.io.Closeable {
/**
* The underlying SocketImpl
*/
// the underlying SocketImpl
private final SocketImpl impl;

/**
* Various states of this socket, need stateLock to change.
*/
// various states
private volatile boolean created; // impl.create(boolean) called
private volatile boolean bound;
private volatile boolean closed;
private final Object stateLock = new Object();

// used to coordinate creating and closing underlying socket
private final Object socketLock = new Object();

/**
* Creates a server socket with the given {@code SocketImpl}.
*/
private ServerSocket(Void unused, SocketImpl impl) {
this.impl = Objects.requireNonNull(impl);
}

AlanBateman marked this conversation as resolved.
Show resolved Hide resolved
/**
* Creates a server socket with a user-specified {@code SocketImpl}.
Expand All @@ -104,9 +109,7 @@ public class ServerSocket implements java.io.Closeable {
* @since 12
*/
protected ServerSocket(SocketImpl impl) {
Objects.requireNonNull(impl);
checkPermission();
this.impl = impl;
this(checkPermission(), impl);
}

private static Void checkPermission() {
Expand Down Expand Up @@ -280,16 +283,26 @@ public ServerSocket(int port, int backlog, InetAddress bindAddr) throws IOExcept
}

/**
* Get the {@code SocketImpl} attached to this socket, creating
* it if necessary.
*
* @return the {@code SocketImpl} attached to that ServerSocket.
* @throws SocketException if creation fails.
* @since 1.4
* Create a SocketImpl for a server socket. The SocketImpl is created
* without an underlying socket.
*/
private static SocketImpl createImpl() {
SocketImplFactory factory = ServerSocket.factory;
if (factory != null) {
return factory.createSocketImpl();
} else {
return SocketImpl.createPlatformSocketImpl(true);
}
}

/**
* Returns the {@code SocketImpl} for this ServerSocket, creating the
* underlying socket if required.
* @throws SocketException if creating the underlying socket fails
*/
private SocketImpl getImpl() throws SocketException {
if (!created) {
synchronized (stateLock) {
synchronized (socketLock) {
if (!created) {
if (closed) {
throw new SocketException("Socket is closed");
Expand All @@ -308,18 +321,6 @@ private SocketImpl getImpl() throws SocketException {
return impl;
}

/**
* Create a SocketImpl for a server socket.
*/
private static SocketImpl createImpl() {
SocketImplFactory factory = ServerSocket.factory;
if (factory != null) {
return factory.createSocketImpl();
} else {
return SocketImpl.createPlatformSocketImpl(true);
}
}

/**
*
* Binds the {@code ServerSocket} to a specific address
Expand Down Expand Up @@ -385,15 +386,11 @@ public void bind(SocketAddress endpoint, int backlog) throws IOException {
if (security != null)
security.checkListen(epoint.getPort());

synchronized (stateLock) {
if (closed)
throw new SocketException("Socket is closed");
if (bound)
throw new SocketException("Already bound");
getImpl().bind(epoint.getAddress(), epoint.getPort());
getImpl().listen(backlog);
bound = true;
}
// SocketImpl bind+listen throw if already bound or closed
SocketImpl impl = getImpl();
impl.bind(epoint.getAddress(), epoint.getPort());
impl.listen(backlog);
bound = true;
}

/**
Expand Down Expand Up @@ -581,19 +578,24 @@ public Socket accept() throws IOException {
* @revised 1.4
*/
protected final void implAccept(Socket s) throws IOException {
SocketImpl si = s.impl;
SocketImpl si = s.impl();

// Socket has no SocketImpl
if (si == null) {
si = implAccept();
s.setImpl(si);
s.postAccept();
try {
s.setConnectedImpl(si);
} catch (SocketException e) {
// s has been closed so newly accepted connection needs to be closed
si.closeQuietly();
throw e;
}
return;
}

// Socket has a SOCKS or HTTP SocketImpl, need delegate
if (si instanceof DelegatingSocketImpl) {
si = ((DelegatingSocketImpl) si).delegate();
if (si instanceof DelegatingSocketImpl dsi) {
si = dsi.delegate();
assert si instanceof PlatformSocketImpl;
}

Expand All @@ -609,17 +611,23 @@ protected final void implAccept(Socket s) throws IOException {
if (impl instanceof PlatformSocketImpl) {
SocketImpl psi = platformImplAccept();
si.copyOptionsTo(psi);
s.setImpl(psi);
si.closeQuietly();
try {
s.setConnectedImpl(psi);
} catch (SocketException e) {
// s has been closed so newly accepted connection needs to be closed
psi.closeQuietly();
throw e;
}
} else {
s.impl = null; // temporarily break connection to impl
s.setImpl(null); // temporarily break connection to impl
try {
customImplAccept(si);
} finally {
s.impl = si; // restore connection to impl
s.setImpl(si); // restore connection to impl
}
s.setConnected();
}
s.postAccept();

}

/**
Expand Down Expand Up @@ -736,7 +744,7 @@ private void ensureCompatible(SocketImpl si) throws IOException {
* @revised 1.4
*/
public void close() throws IOException {
synchronized (stateLock) {
synchronized (socketLock) {
if (!closed) {
closed = true;

Expand Down Expand Up @@ -830,8 +838,8 @@ public int getSoTimeout() throws IOException {
throw new SocketException("Socket is closed");
Object o = getImpl().getOption(SocketOptions.SO_TIMEOUT);
/* extra type safety */
if (o instanceof Integer) {
return ((Integer) o).intValue();
if (o instanceof Integer i) {
return i.intValue();
} else {
return 0;
}
Expand Down Expand Up @@ -1010,10 +1018,9 @@ public static synchronized void setSocketFactory(SocketImplFactory fac) throws I
* @since 1.4
* @see #getReceiveBufferSize
*/
public void setReceiveBufferSize (int size) throws SocketException {
if (!(size > 0)) {
public void setReceiveBufferSize(int size) throws SocketException {
if (size <= 0)
throw new IllegalArgumentException("negative receive size");
}
if (isClosed())
throw new SocketException("Socket is closed");
getImpl().setOption(SocketOptions.SO_RCVBUF, size);
Expand Down