DefaultChannelPipeline.removeLast() / removeFirst() does not call handlerRemoved(...) (#9982)

Motivation:

Due a bug in DefaultChannelPipeline we did not call handlerRemoved(...) if the handlers was removed via removeLast() or removeFirst()

Modifications:

- Correctly implement removeLast() / removeFirst()
- Add unit tests

Result:

handlerRemoved(...) always called on removal

transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java

@@ -736,22 +736,58 @@ public class DefaultChannelPipeline implements ChannelPipeline {
 
     @Override
     public ChannelHandler removeFirst() {
+        final DefaultChannelHandlerContext ctx;
+        EventExecutor executor = executor();
+        boolean inEventLoop = executor.inEventLoop();
         synchronized (handlers) {
             if (handlers.isEmpty()) {
                 throw new NoSuchElementException();
             }
-            return handlers.remove(0).handler();
+            int idx = 0;
+
+            ctx = handlers.remove(idx);
+            assert ctx != null;
+
+            if (!inEventLoop) {
+                try {
+                    executor.execute(() -> remove0(ctx));
+                    return ctx.handler();
+                } catch (Throwable cause) {
+                    handlers.add(idx, ctx);
+                    throw cause;
+                }
+            }
         }
+        remove0(ctx);
+        return ctx.handler();
     }
 
     @Override
     public ChannelHandler removeLast() {
+        final DefaultChannelHandlerContext ctx;
+        EventExecutor executor = executor();
+        boolean inEventLoop = executor.inEventLoop();
         synchronized (handlers) {
             if (handlers.isEmpty()) {
-                throw new NoSuchElementException();
+                return null;
+            }
+            int idx = handlers.size() - 1;
+
+            ctx = handlers.remove(idx);
+            assert ctx != null;
+
+            if (!inEventLoop) {
+                try {
+                    executor.execute(() -> remove0(ctx));
+                    return ctx.handler();
+                } catch (Throwable cause) {
+                    handlers.add(idx, ctx);
+                    throw cause;
+                }
             }
-            return handlers.remove(handlers.size() - 1).handler();
         }
+        remove0(ctx);
+        return ctx.handler();
     }
 
     @Override

transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java

@@ -709,18 +709,52 @@ public class DefaultChannelPipelineTest {
     @Test(timeout = 2000)
     public void testAddRemoveHandlerCalled() throws Throwable {
         ChannelPipeline pipeline = newLocalChannel().pipeline();
-        CallbackCheckHandler handler = new CallbackCheckHandler();
 
+        CallbackCheckHandler handler = new CallbackCheckHandler();
         pipeline.addFirst(handler);
         pipeline.remove(handler);
-
         assertTrue(handler.addedHandler.get());
         assertTrue(handler.removedHandler.get());
 
+        CallbackCheckHandler handlerType = new CallbackCheckHandler();
+        pipeline.addFirst(handlerType);
+        pipeline.remove(handlerType.getClass());
+        assertTrue(handlerType.addedHandler.get());
+        assertTrue(handlerType.removedHandler.get());
+
+        CallbackCheckHandler handlerName = new CallbackCheckHandler();
+        pipeline.addFirst("handler", handlerName);
+        pipeline.remove("handler");
+        assertTrue(handlerName.addedHandler.get());
+        assertTrue(handlerName.removedHandler.get());
+
+        CallbackCheckHandler first = new CallbackCheckHandler();
+        pipeline.addFirst(first);
+        pipeline.removeFirst();
+        assertTrue(first.addedHandler.get());
+        assertTrue(first.removedHandler.get());
+
+        CallbackCheckHandler last = new CallbackCheckHandler();
+        pipeline.addFirst(last);
+        pipeline.removeLast();
+        assertTrue(last.addedHandler.get());
+        assertTrue(last.removedHandler.get());
+
         pipeline.channel().register().syncUninterruptibly();
         Throwable cause = handler.error.get();
+        Throwable causeName = handlerName.error.get();
+        Throwable causeType = handlerType.error.get();
+        Throwable causeFirst = first.error.get();
+        Throwable causeLast = last.error.get();
         pipeline.channel().close().syncUninterruptibly();
+        rethrowIfNotNull(cause);
+        rethrowIfNotNull(causeName);
+        rethrowIfNotNull(causeType);
+        rethrowIfNotNull(causeFirst);
+        rethrowIfNotNull(causeLast);
+    }
 
+    private static void rethrowIfNotNull(Throwable cause) throws Throwable {
         if (cause != null) {
             throw cause;
         }