Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make sure promise finishes during graceful shutdowns #1803

Merged
merged 2 commits into from
Aug 20, 2024
Merged

Make sure promise finishes during graceful shutdowns #1803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83713f9
During graceful shutdowns make sure to still finish promise even if s…
jguerra Aug 19, 2024
378a869
Fix import order
jguerra Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientConnectionsShutdown.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 -> {
Copy link
Collaborator

@argha-c argha-c Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be necessary.

The graceful close promise will not finish if some channels error out while closing.

This sounds incorrect. The closeFuture should be invoked even if certain channels error out on close(). It has the listener wired to set a success on the Promise. Where do you see this not being set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was behavior I suspected while validating our signal handling, but then also verified with a unit test. If one of the handlers in the group throws during close() handling then the closeFuture is not completed, and the channel remains in the group

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really odd. Anyway, this doesn't hurt 🚢

if (!future.isSuccess()) {
LOG.error("Failed to close all connections", future.cause());
}
if (!promise.isDone()) {
promise.setSuccess(null);
}
});
},
GRACEFUL_CLOSE_TIMEOUT.get(),
TimeUnit.SECONDS);
Expand Down
39 changes: 39 additions & 0 deletions zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientConnectionsShutdownTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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";
Expand Down
Loading