From 1b3d7f532562d0c6f565dcf0cae92d63d463a3bf Mon Sep 17 00:00:00 2001
From: Trustin Lee <t@motd.kr>
Date: Wed, 24 Apr 2013 19:25:43 +0900
Subject: [PATCH] Make sure handlerAdded() is called before forwarding the
 buffer content of the removed handler

- Added a test case that reproduces the problem in ReplayingDecoderTest
- Call newHandler.handlerAdded() *before* oldHandler.handlerRemoved() to ensure newHandlerAdded() is called before forwarding the buffer content of the old handler in replace0().
---
 .../handler/codec/ReplayingDecoderTest.java   |  2 --
 .../netty/channel/DefaultChannelPipeline.java | 21 ++++++++++++-------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/codec/src/test/java/io/netty/handler/codec/ReplayingDecoderTest.java b/codec/src/test/java/io/netty/handler/codec/ReplayingDecoderTest.java
index b48e115204..4f4069f345 100644
--- a/codec/src/test/java/io/netty/handler/codec/ReplayingDecoderTest.java
+++ b/codec/src/test/java/io/netty/handler/codec/ReplayingDecoderTest.java
@@ -22,7 +22,6 @@ import io.netty.buffer.Unpooled;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInboundByteHandlerAdapter;
 import io.netty.channel.embedded.EmbeddedByteChannel;
-import org.junit.Ignore;
 import org.junit.Test;
 
 import static org.junit.Assert.*;
@@ -65,7 +64,6 @@ public class ReplayingDecoderTest {
     }
 
     @Test
-    @Ignore("needs a fix")
     public void testReplacement() throws Exception {
         EmbeddedByteChannel ch = new EmbeddedByteChannel(new BloatedLineDecoder());
 
diff --git a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java
index 03300415d2..4605e39631 100755
--- a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java
+++ b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java
@@ -442,25 +442,32 @@ final class DefaultChannelPipeline implements ChannelPipeline {
         return ctx.handler();
     }
 
-    private void replace0(DefaultChannelHandlerContext ctx, String newName,
+    private void replace0(DefaultChannelHandlerContext oldCtx, String newName,
                           DefaultChannelHandlerContext newCtx) {
         checkMultiplicity(newCtx);
 
-        DefaultChannelHandlerContext prev = ctx.prev;
-        DefaultChannelHandlerContext next = ctx.next;
+        DefaultChannelHandlerContext prev = oldCtx.prev;
+        DefaultChannelHandlerContext next = oldCtx.next;
         newCtx.prev = prev;
         newCtx.next = next;
+
+        // Finish the replacement of oldCtx with newCtx in the linked list.
+        // Note that this doesn't mean events will be sent to the new handler immediately
+        // because we are currently at the event handler thread and no more than one handler methods can be invoked
+        // at the same time (we ensured that in replace().)
         prev.next = newCtx;
         next.prev = newCtx;
 
-        if (!ctx.name().equals(newName)) {
-            name2ctx.remove(ctx.name());
+        if (!oldCtx.name().equals(newName)) {
+            name2ctx.remove(oldCtx.name());
         }
         name2ctx.put(newName, newCtx);
 
-        // remove old and add new
-        callHandlerRemoved(ctx, newCtx, newCtx);
+        // Invoke newHandler.handlerAdded() first (i.e. before oldHandler.handlerRemoved() is invoked)
+        // because callHandlerRemoved() will trigger inboundBufferUpdated() or flush() on newHandler and those
+        // event handlers must be called after handlerAdded().
         callHandlerAdded(newCtx);
+        callHandlerRemoved(oldCtx, newCtx, newCtx);
     }
 
     private static void checkMultiplicity(ChannelHandlerContext ctx) {