From 4aa800259674bab303edd9427802574fcb4a3131 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Tue, 23 May 2017 22:08:45 +0200
Subject: [PATCH] Not add ChannelHandler to ChannelPipeline once the pipeline
 was destroyed.

Motivation:

ChannelPipeline will happily add a handler to a closed Channel's pipeline and will call handlerAdded(...) but will not call handlerRemoved(...).

Modifications:

Check if pipeline was destroyed and if so not add the handler at all but propergate an exception.

Result:

Fixes [#6768]
---
 .../netty/channel/DefaultChannelPipeline.java | 13 ++++++++
 .../channel/DefaultChannelPipelineTest.java   | 32 +++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java
index 920790e365..326a8a47a9 100644
--- a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java
+++ b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java
@@ -67,6 +67,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
     private Map<EventExecutorGroup, EventExecutor> childExecutors;
     private MessageSizeEstimator.Handle estimatorHandle;
     private boolean firstRegistration = true;
+    private boolean destroyed;
 
     /**
      * This is the head of a linked list that is processed by {@link #callHandlerAddedForAllHandlers()} and so process
@@ -138,6 +139,12 @@ public class DefaultChannelPipeline implements ChannelPipeline {
         return channel;
     }
 
+    private void checkDestroyed(ChannelHandler handler) {
+        if (destroyed) {
+            throw new ChannelPipelineException("Channel already closed, can not add handler " + handler);
+        }
+    }
+
     @Override
     public final ChannelPipeline addFirst(String name, ChannelHandler handler) {
         return addFirst(null, name, handler);
@@ -147,6 +154,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
     public final ChannelPipeline addFirst(EventExecutorGroup group, String name, ChannelHandler handler) {
         final AbstractChannelHandlerContext newCtx;
         synchronized (this) {
+            checkDestroyed(handler);
             checkMultiplicity(handler);
             name = filterName(name, handler);
 
@@ -196,6 +204,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
     public final ChannelPipeline addLast(EventExecutorGroup group, String name, ChannelHandler handler) {
         final AbstractChannelHandlerContext newCtx;
         synchronized (this) {
+            checkDestroyed(handler);
             checkMultiplicity(handler);
 
             newCtx = newContext(group, filterName(name, handler), handler);
@@ -246,6 +255,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
         final AbstractChannelHandlerContext newCtx;
         final AbstractChannelHandlerContext ctx;
         synchronized (this) {
+            checkDestroyed(handler);
             checkMultiplicity(handler);
             name = filterName(name, handler);
             ctx = getContextOrDie(baseName);
@@ -306,6 +316,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
         final AbstractChannelHandlerContext ctx;
 
         synchronized (this) {
+            checkDestroyed(handler);
             checkMultiplicity(handler);
             name = filterName(name, handler);
             ctx = getContextOrDie(baseName);
@@ -516,6 +527,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
 
         final AbstractChannelHandlerContext newCtx;
         synchronized (this) {
+            checkDestroyed(newHandler);
             checkMultiplicity(newHandler);
             if (newName == null) {
                 newName = generateName(newHandler);
@@ -838,6 +850,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
      * See: https://github.com/netty/netty/issues/3156
      */
     private synchronized void destroy() {
+        destroyed = true;
         destroyUp(head.next, false);
     }
 
diff --git a/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java b/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java
index bd0727296f..a871cb4f62 100644
--- a/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java
+++ b/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java
@@ -1073,6 +1073,38 @@ public class DefaultChannelPipelineTest {
         group.shutdownGracefully(0, 0, TimeUnit.SECONDS);
     }
 
+    @Test
+    public void testHandlerAfterClose() throws Exception {
+        EmbeddedChannel channel = new EmbeddedChannel();
+        channel.close().syncUninterruptibly();
+
+        assertFalse(channel.isActive());
+
+        final AtomicBoolean addedHandler = new AtomicBoolean();
+        final AtomicBoolean removedHandler = new AtomicBoolean();
+
+        ChannelPipeline pipeline = channel.pipeline();
+        try {
+            pipeline.addLast(new ChannelHandlerAdapter() {
+                @Override
+                public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
+                    addedHandler.set(true);
+                }
+
+                @Override
+                public void handlerRemoved(ChannelHandlerContext ctx) throws Exception {
+                    removedHandler.set(true);
+                }
+            });
+            fail();
+        } catch (ChannelPipelineException e) {
+            // expected
+        }
+
+        assertFalse(addedHandler.get());
+        assertFalse(removedHandler.get());
+    }
+
     @Test(timeout = 3000)
     public void testVoidPromiseNotify() throws Throwable {
         ChannelPipeline pipeline1 = new LocalChannel().pipeline();