Repository: netty/netty
Issue: ApplicationProtocolNegotiationHandler loses its place in the context too soon
Labels: defect

### Expected behavior
```
	@Override
	protected void configurePipeline(ChannelHandlerContext ctx, String protocol)
			throws Exception {
		ctx.pipeline().replace(this, null, initializer);
		// or
		ctx.pipeline().addAfter(this, null, initializer);
		// or
		ctx.pipeline().addBefore(this, null, initializer);
	}
```
should work, similarly to how a ```ChannelInitializer``` works.

### Actual behavior
An exception is thrown on all "relative" pipeline operations, because the ApplicationProtocolNegotiationHandler instance has already been removed from the pipeline BEFORE ```configurePipeline()``` is called:

```ApplicationProtocolNegotiationHandler.java

    @Override
    public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {
        if (evt instanceof SslHandshakeCompletionEvent) {
            ctx.pipeline().remove(this); // <-- Removed before calling configurePipeline :-(

            SslHandshakeCompletionEvent handshakeEvent = (SslHandshakeCompletionEvent) evt;
            if (handshakeEvent.isSuccess()) {
                SslHandler sslHandler = ctx.pipeline().get(SslHandler.class);
                if (sslHandler == null) {
                    throw new IllegalStateException("cannot find a SslHandler in the pipeline (requir
ed for " +
                                                    "application-level protocol negotiation)");
                }
                String protocol = sslHandler.applicationProtocol();
                configurePipeline(ctx, protocol != null? protocol : fallbackProtocol);
            } else {
                handshakeFailure(ctx, handshakeEvent.cause());
            }
        }

        ctx.fireUserEventTriggered(evt);
    }
```

Compare that to Channelinitializer:

```ChannelInitializer.java

    @SuppressWarnings("unchecked")
    private boolean initChannel(ChannelHandlerContext ctx) throws Exception {
        if (initMap.add(ctx)) { // Guard against re-entrance.
            try {
                initChannel((C) ctx.channel());
            } catch (Throwable cause) {
                // Explicitly call exceptionCaught(...) as we removed the handler before calling init
Channel(...).
                // We do so to prevent multiple calls to initChannel(...).
                exceptionCaught(ctx, cause);
            } finally {
                ChannelPipeline pipeline = ctx.pipeline();
                if (pipeline.context(this) != null) {
                    pipeline.remove(this);
                }
            }
            return true;
        }
        return false;
    }
```

Note that the comment regarding the handler already having been removed in ```Channelinitializer``` is incorrect. See the following excerpt from current code:

```ChannelInitializer.java

    /**
     * {@inheritDoc} If override this method ensure you call super!
     */
    @Override
    public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
        if (ctx.channel().isRegistered()) {
            // This should always be true with our current DefaultChannelPipeline implementation.
            // The good thing about calling initChannel(...) in handlerAdded(...) is that there will be no ordering
            // surprises if a ChannelInitializer will add another ChannelInitializer. This is as all handlers
            // will be added in the expected order.
            if (initChannel(ctx)) {

                // We are done with init the Channel, removing the initializer now.
                removeState(ctx);
            }
        }
    }
```
### Steps to reproduce
Use ```ctx.replace(...)``` in the ```configurePipeline()``` method.

### Minimal yet complete reproducer code (or URL to code)
Not available.

### Netty version
netty-4.1.36.Final

### JVM version (e.g. `java -version`)
java version "11.0.1" 2018-10-16 LTS
Java(TM) SE Runtime Environment 18.9 (build 11.0.1+13-LTS)
Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11.0.1+13-LTS, mixed mode)

### OS version (e.g. `uname -a`)
Linux nemesis 4.15.0-48-generic #51-Ubuntu SMP Wed Apr 3 08:28:49 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux


Issue comments:
Since ```ApplicationProtocolNegotiationHandler``` is just conceptually a special case of a ```Channelinitializer```, similar logic should be used when invoking ```configurePipeline()```, and patterns possible in ```ChannelInitializer``` should also be possible in ```ApplicationProtocolNegotiationHandler```. The only real difference is that the invocation is not triggered by the handler being added, but rather some event a bit further down the line.
@RoganDawes sounds like a bug... are you interested in providing a PR ?
Will do.

On Tue, 07 May 2019 at 10:59 Norman Maurer <notifications@github.com> wrote:

> @RoganDawes <https://github.com/RoganDawes> sounds like a bug... are you
> interested in providing a PR ?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/netty/netty/issues/9131#issuecomment-489995834>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AABHBC4XOG674D6N6O5EWDLPUFAFHANCNFSM4HLGWTFQ>
> .
>


Pull request review comments:
can you add `{ ...}` to match our code style ? 
can you add `{ ...}` to match our code style ? 
please use `await(...)` and not `getCount`. if you really not want to call `await(...)` just replace the latch usage with an `AtomicBoolean`. 
I didn't want to use ```await(...)``` because it would introduce a 5 second delay on each invocation. An ```AtomicBoolean``` solves that problem nicely, thanks.
Resolved by using ```AtomicBoolean```

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java b/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java
index 0e3ea0f39a3..3df8cf5cb46 100644
--- a/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java
@@ -79,22 +79,29 @@ protected ApplicationProtocolNegotiationHandler(String fallbackProtocol) {
     @Override
     public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {
         if (evt instanceof SslHandshakeCompletionEvent) {
-            ctx.pipeline().remove(this);
 
-            SslHandshakeCompletionEvent handshakeEvent = (SslHandshakeCompletionEvent) evt;
-            if (handshakeEvent.isSuccess()) {
-                SslHandler sslHandler = ctx.pipeline().get(SslHandler.class);
-                if (sslHandler == null) {
-                    throw new IllegalStateException("cannot find a SslHandler in the pipeline (required for " +
-                                                    "application-level protocol negotiation)");
+            try {
+                SslHandshakeCompletionEvent handshakeEvent = (SslHandshakeCompletionEvent) evt;
+                if (handshakeEvent.isSuccess()) {
+                    SslHandler sslHandler = ctx.pipeline().get(SslHandler.class);
+                    if (sslHandler == null) {
+                        throw new IllegalStateException("cannot find a SslHandler in the pipeline (required for "
+                                + "application-level protocol negotiation)");
+                    }
+                    String protocol = sslHandler.applicationProtocol();
+                    configurePipeline(ctx, protocol != null ? protocol : fallbackProtocol);
+                } else {
+                    handshakeFailure(ctx, handshakeEvent.cause());
+                }
+            } catch (Throwable cause) {
+                exceptionCaught(ctx, cause);
+            } finally {
+                ChannelPipeline pipeline = ctx.pipeline();
+                if (pipeline.context(this) != null) {
+                    pipeline.remove(this);
                 }
-                String protocol = sslHandler.applicationProtocol();
-                configurePipeline(ctx, protocol != null? protocol : fallbackProtocol);
-            } else {
-                handshakeFailure(ctx, handshakeEvent.cause());
             }
         }
-
         ctx.fireUserEventTriggered(evt);
     }
 
@@ -119,6 +126,7 @@ protected void handshakeFailure(ChannelHandlerContext ctx, Throwable cause) thro
     @Override
     public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
         logger.warn("{} Failed to select the application-level protocol:", ctx.channel(), cause);
+        ctx.fireExceptionCaught(cause);
         ctx.close();
     }
 }


Test patch:
diff --git a/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java
index 8d8003d745e..17d2a6e0e42 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java
@@ -16,6 +16,32 @@
 
 package io.netty.handler.ssl;
 
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
+
+import java.io.File;
+import java.net.InetSocketAddress;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+
+import javax.net.ssl.SSLEngine;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
 import io.netty.bootstrap.Bootstrap;
 import io.netty.bootstrap.ServerBootstrap;
 import io.netty.buffer.ByteBuf;
@@ -44,28 +70,10 @@
 import io.netty.util.Mapping;
 import io.netty.util.ReferenceCountUtil;
 import io.netty.util.ReferenceCounted;
-import io.netty.util.internal.ResourcesUtil;
 import io.netty.util.concurrent.Promise;
 import io.netty.util.internal.ObjectUtil;
+import io.netty.util.internal.ResourcesUtil;
 import io.netty.util.internal.StringUtil;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-import java.io.File;
-import java.net.InetSocketAddress;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicReference;
-
-import javax.net.ssl.SSLEngine;
-
-import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.CoreMatchers.nullValue;
-import static org.junit.Assert.*;
-import static org.junit.Assume.assumeTrue;
 
 @RunWith(Parameterized.class)
 public class SniHandlerTest {
@@ -339,6 +347,8 @@ public void testSniWithApnHandler() throws Exception {
         SslContext sniContext = makeSslContext(provider, true);
         final SslContext clientContext = makeSslClientContext(provider, true);
         try {
+            final AtomicBoolean serverApnCtx = new AtomicBoolean(false);
+            final AtomicBoolean clientApnCtx = new AtomicBoolean(false);
             final CountDownLatch serverApnDoneLatch = new CountDownLatch(1);
             final CountDownLatch clientApnDoneLatch = new CountDownLatch(1);
 
@@ -363,6 +373,8 @@ protected void initChannel(Channel ch) throws Exception {
                         p.addLast(new ApplicationProtocolNegotiationHandler("foo") {
                             @Override
                             protected void configurePipeline(ChannelHandlerContext ctx, String protocol) {
+                                // addresses issue #9131
+                                serverApnCtx.set(ctx.pipeline().context(this) != null);
                                 serverApnDoneLatch.countDown();
                             }
                         });
@@ -381,6 +393,8 @@ protected void initChannel(Channel ch) throws Exception {
                         ch.pipeline().addLast(new ApplicationProtocolNegotiationHandler("foo") {
                             @Override
                             protected void configurePipeline(ChannelHandlerContext ctx, String protocol) {
+                                // addresses issue #9131
+                                clientApnCtx.set(ctx.pipeline().context(this) != null);
                                 clientApnDoneLatch.countDown();
                             }
                         });
@@ -395,6 +409,8 @@ protected void configurePipeline(ChannelHandlerContext ctx, String protocol) {
 
                 assertTrue(serverApnDoneLatch.await(5, TimeUnit.SECONDS));
                 assertTrue(clientApnDoneLatch.await(5, TimeUnit.SECONDS));
+                assertTrue(serverApnCtx.get());
+                assertTrue(clientApnCtx.get());
                 assertThat(handler.hostname(), is("sni.fake.site"));
                 assertThat(handler.sslContext(), is(sniContext));
             } finally {