From 0a1786c32cb5d4ad3832c6072ff396873ee9dbc6 Mon Sep 17 00:00:00 2001 From: RoganDawes Date: Mon, 13 May 2019 13:49:17 +0200 Subject: [PATCH] Remove the Handler only after it has initialized the channel (#9132) Motivation: Previously, any 'relative' pipeline operations, such as ctx.pipeline().replace(), .addBefore(), addAfter(), etc would fail as the handler was not present in the pipeline. Modification: Used the pattern from ChannelInitializer when invoking configurePipeline(). Result: Fixes #9131 --- ...ApplicationProtocolNegotiationHandler.java | 32 +++++++---- .../io/netty/handler/ssl/SniHandlerTest.java | 57 ++++++++++++------- 2 files changed, 57 insertions(+), 32 deletions(-) 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 2155b3d41e..7cec73cc26 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandler.java @@ -80,22 +80,29 @@ public abstract class ApplicationProtocolNegotiationHandler implements ChannelIn @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); } @@ -120,6 +127,7 @@ public abstract class ApplicationProtocolNegotiationHandler implements ChannelIn @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(); } } 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 1da3f353b1..42805b4113 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,33 @@ package io.netty.handler.ssl; +import static java.util.Objects.requireNonNull; +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; @@ -45,28 +72,10 @@ import io.netty.util.DomainNameMappingBuilder; 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.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 java.util.Objects.requireNonNull; -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 { @@ -340,6 +349,8 @@ public class SniHandlerTest { 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); @@ -364,6 +375,8 @@ public class SniHandlerTest { 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(); } }); @@ -382,6 +395,8 @@ public class SniHandlerTest { 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(); } }); @@ -396,6 +411,8 @@ public class SniHandlerTest { 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 {