diff --git a/netty/src/main/java/io/grpc/netty/NettyServer.java b/netty/src/main/java/io/grpc/netty/NettyServer.java index a1f534770c..ec1dfc2c91 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServer.java +++ b/netty/src/main/java/io/grpc/netty/NettyServer.java @@ -288,10 +288,11 @@ class NettyServer implements InternalServer, InternalWithLogId { if (stats != null) { channelz.removeListenSocket(stats); } + sharedResourceReferenceCounter.release(); + protocolNegotiator.close(); synchronized (NettyServer.this) { listener.serverShutdown(); } - sharedResourceReferenceCounter.release(); } }); try { diff --git a/netty/src/main/java/io/grpc/netty/ProtocolNegotiator.java b/netty/src/main/java/io/grpc/netty/ProtocolNegotiator.java index c3c668d2b1..7dad1af20a 100644 --- a/netty/src/main/java/io/grpc/netty/ProtocolNegotiator.java +++ b/netty/src/main/java/io/grpc/netty/ProtocolNegotiator.java @@ -38,9 +38,10 @@ interface ProtocolNegotiator { ChannelHandler newHandler(GrpcHttp2ConnectionHandler grpcHandler); /** - * Releases resources held by this negotiator. Called when the Channel transitions to terminated. - * Is currently only supported on client-side; server-side protocol negotiators will not see this - * method called. + * Releases resources held by this negotiator. Called when the Channel transitions to terminated + * or when InternalServer is shutdown (depending on client or server). That means handlers + * returned by {@link #newHandler()} can outlive their parent negotiator on server-side, but not + * on client-side. */ void close(); } diff --git a/netty/src/test/java/io/grpc/netty/NettyServerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerTest.java index 174ca1e0e0..f1f96791cd 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerTest.java @@ -36,14 +36,17 @@ import io.grpc.internal.ServerTransportListener; import io.grpc.internal.SharedResourcePool; import io.grpc.internal.TransportTracer; import io.netty.channel.Channel; +import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelOption; import io.netty.channel.WriteBufferWaterMark; +import io.netty.util.AsciiString; import java.net.InetSocketAddress; import java.net.Socket; import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import org.junit.Test; import org.junit.runner.RunWith; @@ -54,8 +57,26 @@ public class NettyServerTest { private final InternalChannelz channelz = new InternalChannelz(); @Test - public void getPort() throws Exception { + public void startStop() throws Exception { InetSocketAddress addr = new InetSocketAddress(0); + + class TestProtocolNegotiator implements ProtocolNegotiator { + boolean closed; + + @Override public ChannelHandler newHandler(GrpcHttp2ConnectionHandler handler) { + throw new UnsupportedOperationException(); + } + + @Override public void close() { + closed = true; + } + + @Override public AsciiString scheme() { + return Utils.HTTP; + } + } + + TestProtocolNegotiator protocolNegotiator = new TestProtocolNegotiator(); NettyServer ns = new NettyServer( addr, Utils.DEFAULT_SERVER_CHANNEL_FACTORY, @@ -63,7 +84,7 @@ public class NettyServerTest { SharedResourcePool.forResource(Utils.DEFAULT_BOSS_EVENT_LOOP_GROUP), SharedResourcePool.forResource(Utils.DEFAULT_WORKER_EVENT_LOOP_GROUP), SharedResourcePool.forResource(Utils.BYTE_BUF_ALLOCATOR), - ProtocolNegotiators.plaintext(), + protocolNegotiator, Collections.emptyList(), TransportTracer.getDefaultFactory(), 1, // ignore @@ -75,6 +96,7 @@ public class NettyServerTest { 1, 1, // ignore true, 0, // ignore channelz); + final SettableFuture serverShutdownCalled = SettableFuture.create(); ns.start(new ServerListener() { @Override public ServerTransportListener transportCreated(ServerTransport transport) { @@ -82,7 +104,9 @@ public class NettyServerTest { } @Override - public void serverShutdown() {} + public void serverShutdown() { + serverShutdownCalled.set(null); + } }); // Check that we got an actual port. @@ -90,6 +114,9 @@ public class NettyServerTest { // Cleanup ns.shutdown(); + // serverShutdown() signals that resources are freed + serverShutdownCalled.get(1, TimeUnit.SECONDS); + assertThat(protocolNegotiator.closed).isTrue(); } @Test