From 83713f94fef0c99f598c306b6ab87f36b5fe6c09 Mon Sep 17 00:00:00 2001 From: Justin Guerra Date: Mon, 19 Aug 2024 16:46:16 -0600 Subject: [PATCH 1/2] During graceful shutdowns make sure to still finish promise even if some channels fail to close --- .../server/ClientConnectionsShutdown.java | 12 +++- .../server/ClientConnectionsShutdownTest.java | 66 +++++++++++++++---- 2 files changed, 63 insertions(+), 15 deletions(-) diff --git a/zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientConnectionsShutdown.java b/zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientConnectionsShutdown.java index 241ed598cb..141c896021 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientConnectionsShutdown.java +++ b/zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientConnectionsShutdown.java @@ -30,10 +30,11 @@ import io.netty.util.concurrent.EventExecutor; import io.netty.util.concurrent.Promise; import io.netty.util.concurrent.ScheduledFuture; -import java.util.concurrent.TimeUnit; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.concurrent.TimeUnit; + /** * TODO: Change this class to be an instance per-port. * So that then the configuration can be different per-port, which is need for the combined FTL/Cloud clusters. @@ -113,7 +114,14 @@ Promise gracefullyShutdownClientChannels(boolean forceCloseAfterTimeout) { ScheduledFuture timeoutTask = executor.schedule( () -> { LOG.warn("Force closing remaining {} active client channels.", channels.size()); - channels.close(); + channels.close().addListener(future -> { + if (!future.isSuccess()) { + LOG.error("Failed to close all connections", future.cause()); + } + if (!promise.isDone()) { + promise.setSuccess(null); + } + }); }, GRACEFUL_CLOSE_TIMEOUT.get(), TimeUnit.SECONDS); diff --git a/zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientConnectionsShutdownTest.java b/zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientConnectionsShutdownTest.java index dab3461d2e..c2b3316a15 100644 --- a/zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientConnectionsShutdownTest.java +++ b/zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientConnectionsShutdownTest.java @@ -16,15 +16,6 @@ package com.netflix.zuul.netty.server; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.ArgumentMatchers.isA; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; - import com.netflix.appinfo.InstanceInfo.InstanceStatus; import com.netflix.config.ConfigurationManager; import com.netflix.discovery.EurekaClient; @@ -34,7 +25,10 @@ import io.netty.bootstrap.ServerBootstrap; import io.netty.channel.Channel; import io.netty.channel.ChannelFuture; +import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInitializer; +import io.netty.channel.ChannelOutboundHandlerAdapter; +import io.netty.channel.ChannelPromise; import io.netty.channel.DefaultEventLoop; import io.netty.channel.DefaultEventLoopGroup; import io.netty.channel.group.ChannelGroup; @@ -44,10 +38,6 @@ import io.netty.channel.local.LocalServerChannel; import io.netty.util.concurrent.EventExecutor; import io.netty.util.concurrent.Promise; -import java.util.UUID; -import java.util.concurrent.Callable; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; import org.apache.commons.configuration.AbstractConfiguration; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -57,6 +47,21 @@ import org.mockito.ArgumentMatchers; import org.mockito.Mockito; +import java.util.UUID; +import java.util.concurrent.Callable; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; + /** * @author Justin Guerra * @since 2/28/23 @@ -175,6 +180,41 @@ void connectionNeedsToBeForceClosed() throws Exception { } } + @Test + void connectionNeedsToBeForceClosedAndOneChannelThrowsAnException() throws Exception { + String configName = "server.outofservice.close.timeout"; + AbstractConfiguration configuration = ConfigurationManager.getConfigInstance(); + + try { + configuration.setProperty(configName, "0"); + createChannels(5); + ChannelFuture connect = new Bootstrap() + .group(CLIENT_EVENT_LOOP) + .channel(LocalChannel.class) + .handler(new ChannelInitializer<>() { + @Override + protected void initChannel(Channel ch) { + ch.pipeline().addLast(new ChannelOutboundHandlerAdapter() { + @Override + public void close(ChannelHandlerContext ctx, ChannelPromise promise) throws Exception { + throw new Exception(); + } + }); + } + }) + .remoteAddress(LOCAL_ADDRESS) + .connect() + .sync(); + channels.add(connect.channel()); + + boolean await = shutdown.gracefullyShutdownClientChannels().await(10, TimeUnit.SECONDS); + assertTrue(await, "the promise should finish even if a channel failed to close"); + assertEquals(1, channels.size(), "all other channels should have been closed"); + } finally { + configuration.setProperty(configName, "30"); + } + } + @Test void connectionsNotForceClosed() throws Exception { String configName = "server.outofservice.close.timeout"; From 378a8693eb39892446096e7a5305532b1346d6da Mon Sep 17 00:00:00 2001 From: Justin Guerra Date: Mon, 19 Aug 2024 16:47:51 -0600 Subject: [PATCH 2/2] Fix import order --- .../server/ClientConnectionsShutdown.java | 3 +- .../server/ClientConnectionsShutdownTest.java | 29 +++++++++---------- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientConnectionsShutdown.java b/zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientConnectionsShutdown.java index 141c896021..d651b9b4ab 100644 --- a/zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientConnectionsShutdown.java +++ b/zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientConnectionsShutdown.java @@ -30,11 +30,10 @@ import io.netty.util.concurrent.EventExecutor; import io.netty.util.concurrent.Promise; import io.netty.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.concurrent.TimeUnit; - /** * TODO: Change this class to be an instance per-port. * So that then the configuration can be different per-port, which is need for the combined FTL/Cloud clusters. diff --git a/zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientConnectionsShutdownTest.java b/zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientConnectionsShutdownTest.java index c2b3316a15..28f6df54a5 100644 --- a/zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientConnectionsShutdownTest.java +++ b/zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientConnectionsShutdownTest.java @@ -16,6 +16,16 @@ package com.netflix.zuul.netty.server; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; + import com.netflix.appinfo.InstanceInfo.InstanceStatus; import com.netflix.config.ConfigurationManager; import com.netflix.discovery.EurekaClient; @@ -38,6 +48,10 @@ import io.netty.channel.local.LocalServerChannel; import io.netty.util.concurrent.EventExecutor; import io.netty.util.concurrent.Promise; +import java.util.UUID; +import java.util.concurrent.Callable; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; import org.apache.commons.configuration.AbstractConfiguration; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -47,21 +61,6 @@ import org.mockito.ArgumentMatchers; import org.mockito.Mockito; -import java.util.UUID; -import java.util.concurrent.Callable; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.ArgumentMatchers.isA; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; - /** * @author Justin Guerra * @since 2/28/23