From d446765b8469ca40db40f46e5c637d980b734a8a Mon Sep 17 00:00:00 2001
From: Nitsan Wakart <nitsanw@users.noreply.github.com>
Date: Thu, 5 Sep 2019 09:12:16 +0200
Subject: [PATCH] Replace synchronized blocks + assert with synchronized method
 (#9538)

Motivation:

Following up on discussion with @normanmaurer with suggestion to improve code clarity.

Modification:

Method is synchronized, no need for assert or verbose sync blocks around calls.

Result:

Reduce verbosity and more idiomatic use of keyword. Also rename the method to better describe what it's for.
---
 .../netty/channel/DefaultChannelPipeline.java | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java
index 8a58be3ad1..5b08308879 100644
--- a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java
+++ b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java
@@ -457,7 +457,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
         assert ctx != head && ctx != tail;
 
         synchronized (this) {
-            remove0(ctx);
+            atomicRemoveFromHandlerList(ctx);
 
             // If the registered is false it means that the channel was not registered on an eventloop yet.
             // In this case we remove the context from the pipeline and add a task that will call
@@ -482,8 +482,10 @@ public class DefaultChannelPipeline implements ChannelPipeline {
         return ctx;
     }
 
-    private void remove0(AbstractChannelHandlerContext ctx) {
-        assert Thread.holdsLock(this);
+    /**
+     * Method is synchronized to make the handler removal from the double linked list atomic.
+     */
+    private synchronized void atomicRemoveFromHandlerList(AbstractChannelHandlerContext ctx) {
         AbstractChannelHandlerContext prev = ctx.prev;
         AbstractChannelHandlerContext next = ctx.next;
         prev.next = next;
@@ -612,9 +614,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
         } catch (Throwable t) {
             boolean removed = false;
             try {
-                synchronized (this) {
-                    remove0(ctx);
-                }
+                atomicRemoveFromHandlerList(ctx);
                 ctx.callHandlerRemoved();
                 removed = true;
             } catch (Throwable t2) {
@@ -884,9 +884,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
 
             final EventExecutor executor = ctx.executor();
             if (inEventLoop || executor.inEventLoop(currentThread)) {
-                synchronized (this) {
-                    remove0(ctx);
-                }
+                atomicRemoveFromHandlerList(ctx);
                 callHandlerRemoved0(ctx);
             } else {
                 final AbstractChannelHandlerContext finalCtx = ctx;
@@ -1484,9 +1482,7 @@ public class DefaultChannelPipeline implements ChannelPipeline {
                                 "Can't invoke handlerAdded() as the EventExecutor {} rejected it, removing handler {}.",
                                 executor, ctx.name(), e);
                     }
-                    synchronized (this) {
-                        remove0(ctx);
-                    }
+                    atomicRemoveFromHandlerList(ctx);
                     ctx.setRemoved();
                 }
             }