From cf0259661e7d02d65f18a73a173aaccd9164d188 Mon Sep 17 00:00:00 2001 From: Trustin Lee Date: Mon, 11 Jun 2012 11:53:43 +0900 Subject: [PATCH] Fix a race condition where local channel's closeFuture is notified early - Added AbstractChannel.doPreClose() to allow a transport to perform a task before closeFuture is notified --- .../main/java/io/netty/channel/AbstractChannel.java | 11 ++++++++++- .../java/io/netty/channel/local/LocalChannel.java | 9 +++++++-- .../io/netty/channel/local/LocalServerChannel.java | 8 +++++++- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/transport/src/main/java/io/netty/channel/AbstractChannel.java b/transport/src/main/java/io/netty/channel/AbstractChannel.java index 873b60f1cd..0fc46a0f1f 100644 --- a/transport/src/main/java/io/netty/channel/AbstractChannel.java +++ b/transport/src/main/java/io/netty/channel/AbstractChannel.java @@ -708,6 +708,10 @@ public abstract class AbstractChannel extends DefaultAttributeMap implements Cha protected abstract Runnable doRegister() throws Exception; protected abstract void doBind(SocketAddress localAddress) throws Exception; protected abstract void doDisconnect() throws Exception; + protected void doPreClose() throws Exception { + // NOOP by default + } + protected abstract void doClose() throws Exception; protected abstract void doDeregister() throws Exception; protected void doFlushByteBuffer(ByteBuf buf) throws Exception { @@ -799,7 +803,7 @@ public abstract class AbstractChannel extends DefaultAttributeMap implements Cha } } - private static final class CloseFuture extends DefaultChannelFuture implements ChannelFuture.Unsafe { + private final class CloseFuture extends DefaultChannelFuture implements ChannelFuture.Unsafe { CloseFuture(AbstractChannel ch) { super(ch, false); @@ -816,6 +820,11 @@ public abstract class AbstractChannel extends DefaultAttributeMap implements Cha } boolean setClosed() { + try { + doPreClose(); + } catch (Exception e) { + logger.warn("doPreClose() raised an exception.", e); + } return super.setSuccess(); } } diff --git a/transport/src/main/java/io/netty/channel/local/LocalChannel.java b/transport/src/main/java/io/netty/channel/local/LocalChannel.java index bd4643919b..1e5a981f2c 100644 --- a/transport/src/main/java/io/netty/channel/local/LocalChannel.java +++ b/transport/src/main/java/io/netty/channel/local/LocalChannel.java @@ -17,10 +17,10 @@ package io.netty.channel.local; import io.netty.channel.AbstractChannel; import io.netty.channel.Channel; +import io.netty.channel.ChannelBufferType; import io.netty.channel.ChannelConfig; import io.netty.channel.ChannelException; import io.netty.channel.ChannelFuture; -import io.netty.channel.ChannelBufferType; import io.netty.channel.DefaultChannelConfig; import io.netty.channel.EventLoop; import io.netty.channel.SingleThreadEventLoop; @@ -171,17 +171,22 @@ public class LocalChannel extends AbstractChannel { } @Override - protected void doClose() throws Exception { + protected void doPreClose() throws Exception { if (state > 2) { // Closed already return; } + // Update all internal state before the closeFuture is notified. if (parent() == null) { LocalChannelRegistry.unregister(localAddress); } localAddress = null; state = 3; + } + + @Override + protected void doClose() throws Exception { if (peer.isActive()) { peer.unsafe().close(peer.unsafe().voidFuture()); peer = null; diff --git a/transport/src/main/java/io/netty/channel/local/LocalServerChannel.java b/transport/src/main/java/io/netty/channel/local/LocalServerChannel.java index 7478d9c71e..0aa42207ef 100644 --- a/transport/src/main/java/io/netty/channel/local/LocalServerChannel.java +++ b/transport/src/main/java/io/netty/channel/local/LocalServerChannel.java @@ -96,17 +96,23 @@ public class LocalServerChannel extends AbstractServerChannel { } @Override - protected void doClose() throws Exception { + protected void doPreClose() throws Exception { if (state > 1) { // Closed already. return; } + // Update all internal state before the closeFuture is notified. LocalChannelRegistry.unregister(localAddress); localAddress = null; state = 2; } + @Override + protected void doClose() throws Exception { + // All internal state was updated already at doPreClose(). + } + @Override protected void doDeregister() throws Exception { ((SingleThreadEventLoop) eventLoop()).removeShutdownHook(shutdownHook);