Clear scheduled timeout if channel is closed with incomplete WebSocket handshake (#10510)

Motivation:

Consider a scenario when the client iniitiates a WebSocket handshake but before the handshake is complete,
the channel is closed due to some reason. In such scenario, the handshake timeout scheduled on the executor
is not cleared. The reason it is not cleared is because in such cases the handshakePromise is not completed.

Modifications:

This change completes the handshakePromise exceptinoally on channelInactive callback, if it has not been
completed so far. This triggers the callback on completion of the promise which clears the timeout scheduled
on the executor.

This PR also adds a test case which reproduces the scenario described above. The test case fails before the
fix is added and succeeds when the fix is applied.

Result:

After this change, the timeout scheduled on the executor will be cleared, thus freeing up thread resources.
This commit is contained in:
Divij Vaidya 2020-09-16 00:40:42 -07:00 committed by GitHub
parent 836bb74051
commit 79a7c157a3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 44 additions and 3 deletions

View File

@ -70,6 +70,15 @@ class WebSocketClientProtocolHandshakeHandler extends ChannelInboundHandlerAdapt
applyHandshakeTimeout();
}
@Override
public void channelInactive(ChannelHandlerContext ctx) throws Exception {
if (!handshakePromise.isDone()) {
handshakePromise.tryFailure(new WebSocketHandshakeException("channel closed with handshake in progress"));
}
super.channelInactive(ctx);
}
@Override
public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
if (!(msg instanceof FullHttpResponse)) {

View File

@ -184,6 +184,40 @@ public class WebSocketHandshakeHandOverTest {
}
}
/**
* Tests a scenario when channel is closed while the handshake is in progress. Validates that the handshake
* future is notified in such cases.
*/
@Test
public void testHandshakeFutureIsNotifiedOnChannelClose() throws Exception {
EmbeddedChannel clientChannel = createClientChannel(null);
EmbeddedChannel serverChannel = createServerChannel(null);
try {
// Start handshake from client to server but don't complete the handshake for the purpose of this test.
transferAllDataWithMerge(clientChannel, serverChannel);
final WebSocketClientProtocolHandler clientWsHandler =
clientChannel.pipeline().get(WebSocketClientProtocolHandler.class);
final WebSocketClientProtocolHandshakeHandler clientWsHandshakeHandler =
clientChannel.pipeline().get(WebSocketClientProtocolHandshakeHandler.class);
final ChannelHandlerContext ctx = clientChannel.pipeline().context(WebSocketClientProtocolHandler.class);
// Close the channel while the handshake is in progress. The channel could be closed before the handshake is
// complete due to a number of varied reasons. To reproduce the test scenario for this test case,
// we would manually close the channel.
clientWsHandler.close(ctx, ctx.newPromise());
// At this stage handshake is incomplete but the handshake future should be completed exceptionally since
// channel is closed.
assertTrue(clientWsHandshakeHandler.getHandshakeFuture().isDone());
} finally {
serverChannel.finishAndReleaseAll();
clientChannel.finishAndReleaseAll();
}
}
@Test(timeout = 10000)
public void testClientHandshakerForceClose() throws Exception {
final WebSocketClientHandshaker handshaker = WebSocketClientHandshakerFactory.newHandshaker(
@ -308,9 +342,7 @@ public class WebSocketHandshakeHandOverTest {
}
private static EmbeddedChannel createServerChannel(ChannelHandler handler) {
return new EmbeddedChannel(
new HttpServerCodec(),
new HttpObjectAggregator(8192),
return createServerChannel(
new WebSocketServerProtocolHandler("/test", "test-proto-1, test-proto-2", false),
handler);
}