From c797e7bdc58509eb0bd25c090add8dc5ee9dbb7c Mon Sep 17 00:00:00 2001 From: Trustin Lee Date: Fri, 5 Dec 2014 15:51:40 +0900 Subject: [PATCH] Fix a race condition where handler is removed before unregistration Related: #3156 Motivation: Let's say we have a channel with the following pipeline configuration: HEAD --> [E1] H1 --> [E2] H2 --> TAIL when the channel is deregistered, the channelUnregistered() methods of H1 and H2 will be invoked from the executor thread of E1 and E2 respectively. To ensure that the channelUnregistered() methods are invoked from the correct thread, new one-time tasks will be created accordingly and be scheduled via Executor.execute(Runnable). As soon as the one-time tasks are scheduled, DefaultChannelPipeline.fireChannelUnregistered() will start to remove all handlers from the pipeline via teardownAll(). This process is performed in reversed order of event propagation. i.e. H2 is removed first, and then H1 is removed. If the channelUnregistered() event has been passed to H2 before H2 is removed, a user does not see any problem. If H2 has been removed before channelUnregistered() event is passed to H2, a user will often see the following confusing warning message: An exceptionCaught() event was fired, and it reached at the tail of the pipeline. It usually means the last handler in the pipeline did not handle the exception. Modifications: To ensure that the handlers are removed *after* all events are propagated, traverse the pipeline in ascending order before performing the actual removal. Result: A user does not get the confusing warning message anymore. --- .../AbstractChannelHandlerContext.java | 30 -------- .../netty/channel/DefaultChannelPipeline.java | 70 +++++++++++++++++-- 2 files changed, 64 insertions(+), 36 deletions(-) diff --git a/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java b/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java index cd5f208ec5..69cf681e04 100644 --- a/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java +++ b/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java @@ -24,7 +24,6 @@ import io.netty.util.ResourceLeakHint; import io.netty.util.concurrent.EventExecutor; import io.netty.util.concurrent.FastThreadLocal; import io.netty.util.concurrent.PausableEventExecutor; -import io.netty.util.internal.OneTimeTask; import io.netty.util.internal.PlatformDependent; import io.netty.util.internal.StringUtil; import java.net.SocketAddress; @@ -251,35 +250,6 @@ abstract class AbstractChannelHandlerContext implements ChannelHandlerContext, R this.skipFlags = skipFlags; } - /** Invocation initiated by {@link DefaultChannelPipeline#teardownAll()}}. */ - void teardown() { - EventExecutor executor = executor(); - if (executor.inEventLoop()) { - teardown0(); - } else { - /** - * use unwrap(), because the executor will usually be a {@link PausableEventExecutor} - * that might not accept any new tasks. - */ - executor().unwrap().execute(new OneTimeTask() { - @Override - public void run() { - teardown0(); - } - }); - } - } - - private void teardown0() { - AbstractChannelHandlerContext prev = this.prev; - if (prev != null) { - synchronized (pipeline) { - pipeline.remove0(this); - } - prev.teardown(); - } - } - @Override public final Channel channel() { return channel; diff --git a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java index 4dc241f7cd..8e9526ee9b 100644 --- a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java +++ b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java @@ -849,18 +849,76 @@ final class DefaultChannelPipeline implements ChannelPipeline { // Remove all handlers sequentially if channel is closed and unregistered. if (!channel.isOpen()) { - teardownAll(); + destroy(); } return this; } /** - * Removes all handlers from the pipeline one by one from tail (exclusive) to head (inclusive) to trigger - * handlerRemoved(). Note that the tail handler is excluded because it's neither an outbound handler nor it - * does anything in handlerRemoved(). + * Removes all handlers from the pipeline one by one from tail (exclusive) to head (exclusive) to trigger + * handlerRemoved(). + * + * Note that we traverse up the pipeline ({@link #destroyUp(AbstractChannelHandlerContext)}) + * before traversing down ({@link #destroyDown(Thread, AbstractChannelHandlerContext)}) so that + * the handlers are removed after all events are handled. + * + * See: https://github.com/netty/netty/issues/3156 */ - private void teardownAll() { - tail.prev.teardown(); + private void destroy() { + destroyUp(head.next); + } + + private void destroyUp(AbstractChannelHandlerContext ctx) { + final Thread currentThread = Thread.currentThread(); + final AbstractChannelHandlerContext tail = this.tail; + for (;;) { + if (ctx == tail) { + destroyDown(currentThread, tail.prev); + break; + } + + final EventExecutor executor = ctx.executor(); + if (!executor.inEventLoop(currentThread)) { + final AbstractChannelHandlerContext finalCtx = ctx; + executor.unwrap().execute(new OneTimeTask() { + @Override + public void run() { + destroyUp(finalCtx); + } + }); + break; + } + + ctx = ctx.next; + } + } + + private void destroyDown(Thread currentThread, AbstractChannelHandlerContext ctx) { + // We have reached at tail; now traverse backwards. + final AbstractChannelHandlerContext head = this.head; + for (;;) { + if (ctx == head) { + break; + } + + final EventExecutor executor = ctx.executor(); + if (executor.inEventLoop(currentThread)) { + synchronized (this) { + remove0(ctx); + } + } else { + final AbstractChannelHandlerContext finalCtx = ctx; + executor.unwrap().execute(new OneTimeTask() { + @Override + public void run() { + destroyDown(Thread.currentThread(), finalCtx); + } + }); + break; + } + + ctx = ctx.prev; + } } @Override