From 42d2f79239e8b15ed77ea95b760633df87ad1828 Mon Sep 17 00:00:00 2001 From: Trustin Lee Date: Mon, 30 Nov 2009 11:59:00 +0000 Subject: [PATCH] Resolved issue: NETTY-256 (A race condition during the recommended server shutdown procedure) * Fixed a bug in the socket transport implementations where a new connection can be accepted after the ChannelFuture of ServerSocketChannel.close() is complete. * Introduced a lock to ensure that the boss thread is terminated before notifying the future --- .../socket/nio/NioServerSocketChannel.java | 3 +++ .../nio/NioServerSocketPipelineSink.java | 25 +++++++++++++------ .../socket/oio/OioServerSocketChannel.java | 3 +++ .../oio/OioServerSocketPipelineSink.java | 25 +++++++++++++------ 4 files changed, 42 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/jboss/netty/channel/socket/nio/NioServerSocketChannel.java b/src/main/java/org/jboss/netty/channel/socket/nio/NioServerSocketChannel.java index f6c32f0ac4..59cb5aef29 100644 --- a/src/main/java/org/jboss/netty/channel/socket/nio/NioServerSocketChannel.java +++ b/src/main/java/org/jboss/netty/channel/socket/nio/NioServerSocketChannel.java @@ -20,6 +20,8 @@ import static org.jboss.netty.channel.Channels.*; import java.io.IOException; import java.net.InetSocketAddress; import java.nio.channels.ServerSocketChannel; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import org.jboss.netty.channel.AbstractServerChannel; import org.jboss.netty.channel.ChannelException; @@ -46,6 +48,7 @@ class NioServerSocketChannel extends AbstractServerChannel InternalLoggerFactory.getInstance(NioServerSocketChannel.class); final ServerSocketChannel socket; + final Lock shutdownLock = new ReentrantLock(); private final ServerSocketChannelConfig config; NioServerSocketChannel( diff --git a/src/main/java/org/jboss/netty/channel/socket/nio/NioServerSocketPipelineSink.java b/src/main/java/org/jboss/netty/channel/socket/nio/NioServerSocketPipelineSink.java index afc315eed5..2e11ae34f6 100644 --- a/src/main/java/org/jboss/netty/channel/socket/nio/NioServerSocketPipelineSink.java +++ b/src/main/java/org/jboss/netty/channel/socket/nio/NioServerSocketPipelineSink.java @@ -175,14 +175,23 @@ class NioServerSocketPipelineSink extends AbstractChannelSink { boolean bound = channel.isBound(); try { channel.socket.close(); - if (channel.setClosed()) { - future.setSuccess(); - if (bound) { - fireChannelUnbound(channel); + + // Make sure the boss thread is not running so that that the future + // is notified after a new connection cannot be accepted anymore. + // See NETTY-256 for more information. + channel.shutdownLock.lock(); + try { + if (channel.setClosed()) { + future.setSuccess(); + if (bound) { + fireChannelUnbound(channel); + } + fireChannelClosed(channel); + } else { + future.setSuccess(); } - fireChannelClosed(channel); - } else { - future.setSuccess(); + } finally { + channel.shutdownLock.unlock(); } } catch (Throwable t) { future.setFailure(t); @@ -218,6 +227,7 @@ class NioServerSocketPipelineSink extends AbstractChannelSink { public void run() { final Thread currentThread = Thread.currentThread(); + channel.shutdownLock.lock(); for (;;) { try { if (selector.select(1000) > 0) { @@ -249,6 +259,7 @@ class NioServerSocketPipelineSink extends AbstractChannelSink { } } + channel.shutdownLock.unlock(); closeSelector(); } diff --git a/src/main/java/org/jboss/netty/channel/socket/oio/OioServerSocketChannel.java b/src/main/java/org/jboss/netty/channel/socket/oio/OioServerSocketChannel.java index 8341fe8dd7..b6827bbb58 100644 --- a/src/main/java/org/jboss/netty/channel/socket/oio/OioServerSocketChannel.java +++ b/src/main/java/org/jboss/netty/channel/socket/oio/OioServerSocketChannel.java @@ -20,6 +20,8 @@ import static org.jboss.netty.channel.Channels.*; import java.io.IOException; import java.net.InetSocketAddress; import java.net.ServerSocket; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import org.jboss.netty.channel.AbstractServerChannel; import org.jboss.netty.channel.ChannelException; @@ -47,6 +49,7 @@ class OioServerSocketChannel extends AbstractServerChannel InternalLoggerFactory.getInstance(OioServerSocketChannel.class); final ServerSocket socket; + final Lock shutdownLock = new ReentrantLock(); private final ServerSocketChannelConfig config; OioServerSocketChannel( diff --git a/src/main/java/org/jboss/netty/channel/socket/oio/OioServerSocketPipelineSink.java b/src/main/java/org/jboss/netty/channel/socket/oio/OioServerSocketPipelineSink.java index f84ec1869b..23204291bc 100644 --- a/src/main/java/org/jboss/netty/channel/socket/oio/OioServerSocketPipelineSink.java +++ b/src/main/java/org/jboss/netty/channel/socket/oio/OioServerSocketPipelineSink.java @@ -164,14 +164,23 @@ class OioServerSocketPipelineSink extends AbstractChannelSink { boolean bound = channel.isBound(); try { channel.socket.close(); - if (channel.setClosed()) { - future.setSuccess(); - if (bound) { - fireChannelUnbound(channel); + + // Make sure the boss thread is not running so that that the future + // is notified after a new connection cannot be accepted anymore. + // See NETTY-256 for more information. + channel.shutdownLock.lock(); + try { + if (channel.setClosed()) { + future.setSuccess(); + if (bound) { + fireChannelUnbound(channel); + } + fireChannelClosed(channel); + } else { + future.setSuccess(); } - fireChannelClosed(channel); - } else { - future.setSuccess(); + } finally { + channel.shutdownLock.unlock(); } } catch (Throwable t) { future.setFailure(t); @@ -187,6 +196,7 @@ class OioServerSocketPipelineSink extends AbstractChannelSink { } public void run() { + channel.shutdownLock.lock(); while (channel.isBound()) { try { Socket acceptedSocket = channel.socket.accept(); @@ -238,6 +248,7 @@ class OioServerSocketPipelineSink extends AbstractChannelSink { } } } + channel.shutdownLock.unlock(); } } }