Skip to content

Commit

Permalink
common: Improve exception handling on asynchronously closed channels
Browse files Browse the repository at this point in the history
Previously, we were throwing SocketClosedException in cases where
BrokenPipeSocketException was more appropriate.

This enables us to better differentiate between
AsynchronousCloseException (broken pipe, etc.) and any other
ClosedChannelException.

Adjust the corresponding unit tests, which now always either throw
AsynchronousCloseException or ClosedByInterruptException.

#158
  • Loading branch information
kohlschuetter committed Jul 1, 2024
1 parent 4dde168 commit 44cdadb
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 10 deletions.
10 changes: 10 additions & 0 deletions junixsocket-common/src/main/java/org/newsclub/net/unix/AFCore.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import java.net.SocketException;
import java.net.SocketTimeoutException;
import java.nio.ByteBuffer;
import java.nio.channels.AsynchronousCloseException;
import java.nio.channels.ClosedChannelException;
import java.nio.channels.SelectionKey;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicBoolean;
Expand Down Expand Up @@ -200,6 +202,14 @@ int read(ByteBuffer dst, AFSupplier<Integer> timeout, ByteBuffer socketAddressBu
park = true;
continue virtualThreadLoop;
}
} catch (AsynchronousCloseException e) {
throw e;
} catch (ClosedChannelException e) {
if (isClosed()) {
throw e;
} else {
throw (AsynchronousCloseException) new AsynchronousCloseException().initCause(e);
}
} catch (SocketTimeoutException e) {
if (virtualBlocking) {
// try again
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ AFSocket<A> accept1(boolean throwOnFail) throws IOException {
boolean success = implementation.accept0(as.getAFImpl(false));
if (isClosed()) {
// We may have connected to the socket to unblock it
throw new SocketClosedException("Socket is closed");
throw new BrokenPipeSocketException("Socket is closed");
}

if (!success) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ final boolean accept0(SocketImpl socket) throws IOException {
if (caught != null) {
throw caught;
} else {
throw new SocketClosedException("Socket is closed");
throw new BrokenPipeSocketException("Socket is closed");
}
} else if (caught != null) {
throw caught;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,18 +91,26 @@ private static <T extends Exception> T closeAndThrow(AbstractInterruptibleChanne
static IOException handleException(AbstractInterruptibleChannel channel, IOException e) {
if (e instanceof SocketClosedException || e instanceof ClosedChannelException
|| e instanceof BrokenPipeSocketException) {
Thread t = Thread.currentThread();

if (e instanceof SocketClosedByInterruptException
|| e instanceof ClosedByInterruptException) {
Thread t = Thread.currentThread();
if (!t.isInterrupted()) {
t.interrupt();
}
}

if (!(e instanceof ClosedChannelException)) {
// Make sure the caught exception is transformed into the expected exception
e = (ClosedChannelException) new ClosedChannelException().initCause(e);
if (t.isInterrupted()) {
e = (ClosedByInterruptException) new ClosedByInterruptException().initCause(e);
} else if (e instanceof BrokenPipeSocketException) {
e = (AsynchronousCloseException) new AsynchronousCloseException().initCause(e);
} else {
e = (ClosedChannelException) new ClosedChannelException().initCause(e);
}
}

return closeAndThrow(channel, e);
} else {
return e;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.io.IOException;
import java.net.SocketException;
import java.nio.ByteBuffer;
import java.nio.channels.AsynchronousCloseException;
import java.nio.channels.ClosedByInterruptException;
import java.nio.channels.ClosedChannelException;
import java.nio.channels.ServerSocketChannel;
Expand Down Expand Up @@ -79,12 +80,13 @@ private static List<Arguments> clientProvider() {
socket(true, () -> AFUNIXSocket.connectTo(SOCKET_ADDR), s -> s.getInputStream().read(),
SocketException.class, AFUNIXSocket::isClosed), //
socket(true, () -> AFUNIXSocket.connectTo(SOCKET_ADDR), s -> s.getOutputStream().write(10),
SocketException.class, AFUNIXSocket::isClosed), socket(false, AFUNIXSocketChannel::open,
s -> s.connect(SOCKET_ADDR), ClosedChannelException.class, s -> !s.isOpen()), //
SocketException.class, AFUNIXSocket::isClosed), //
socket(false, AFUNIXSocketChannel::open, s -> s.connect(SOCKET_ADDR),
AsynchronousCloseException.class, s -> !s.isOpen()), //
socket(true, InterruptIssue158Test::connectSocketChannel, s -> s.read(ByteBuffer.allocate(
1)), ClosedChannelException.class, s -> !s.isOpen()), //
1)), AsynchronousCloseException.class, s -> !s.isOpen()), //
socket(true, InterruptIssue158Test::connectSocketChannel, s -> s.write(ByteBuffer.allocate(
1)), ClosedChannelException.class, s -> !s.isOpen()) //
1)), AsynchronousCloseException.class, s -> !s.isOpen()) //
);
}

Expand All @@ -93,7 +95,7 @@ private static List<Arguments> serverProvider() {
serverSocket(() -> AFUNIXServerSocket.bindOn(SOCKET_ADDR), AFUNIXServerSocket::accept,
SocketException.class, AFUNIXServerSocket::isClosed), //
serverSocket(InterruptIssue158Test::bindServerSocketChannel,
AFUNIXServerSocketChannel::accept, ClosedChannelException.class, s -> !s.isOpen())//
AFUNIXServerSocketChannel::accept, AsynchronousCloseException.class, s -> !s.isOpen())//
);
}

Expand Down Expand Up @@ -218,7 +220,7 @@ <T extends AutoCloseable> Throwable runOperation(CountDownLatch ready, IOSupplie
// Also, when we expect any kind of ClosedChannelException, it is only expected to be
// set when the actual exception thrown is from the ClosedByInterruptException subclass.
boolean ignoreInterruptState = SocketException.class.equals(expectedException);
boolean interruptStateOK = Thread.interrupted() || (ClosedChannelException.class.equals(
boolean interruptStateOK = Thread.interrupted() || (AsynchronousCloseException.class.equals(
expectedException) && !(e instanceof ClosedByInterruptException));

assertAll(() -> assertInstanceOf(expectedException, e, "Socket exception"),
Expand Down

0 comments on commit 44cdadb

Please sign in to comment.