Make sure promise finishes during graceful shutdowns (#1803)

* During graceful shutdowns make sure to still finish promise even if some channels fail to close * Fix import order
Netflix · Sep 9, 2024 · 15cbe62 · 15cbe62
1 parent d3efb38
commit 15cbe62
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 1 deletion.
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
@@ -113,7 +113,14 @@ Promise<Void> 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
@@ -16,6 +16,7 @@
 
 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;
@@ -34,7 +35,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;
@@ -175,6 +179,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";