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.

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

Check if pooled connection is still open #95

Merged
merged 3 commits into from Dec 13, 2018

Conversation

Projects
None yet
3 participants
@arhimondr
Contributor

arhimondr commented Nov 9, 2018

Closing a netty channel is an asynchronous operation. Before the connection
is closed, a new request may come in and grab a connection that is about to be
closed by the previous request.

@dain

I have a few questions

// check if connection is failed or closed
try {
Channel channel = future.get();

This comment has been minimized.

@dain

dain Nov 9, 2018

Member

use future.getNow() which doesn't throw

@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
{
// Ignore connection closed kind of exceptions

This comment has been minimized.

@dain

dain Nov 9, 2018

Member

In stead of saying "Ignore", maybe say don't log connection closed... also I think that would be a good change to the commit message

@@ -92,6 +98,17 @@ public void channelRead(ChannelHandlerContext context, Object message)
context.fireChannelRead(message);
}
@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)

This comment has been minimized.

@dain

dain Nov 9, 2018

Member

Is this the right place to put this? The ThriftServerHandler is the inner most handler on the server. Does it get exceptions from the ChannelHandlers closer to the server socket?

This comment has been minimized.

@arhimondr

arhimondr Nov 12, 2018

Contributor

I think the chain is next

LengthFieldBasedFrameDecoder -> SimpleFrameCodec -> ResponseOrderingHandler -> ThriftServerHandler

If something fails in upstream handlers, the error is propagated all the way downstream to the ThriftServerHandler (unless the error is handled somewhere upstream. SimpleFrameCodec and ResponseOrderingHandler don't have the exceptionCaught method, so the error is propagated all the way to the ThriftServerHandler.

This comment has been minimized.

@arhimondr

arhimondr Nov 12, 2018

Contributor

I'm not sure how the write path works though. What is the propagation chain if the error happens when response is written to the client.

This comment has been minimized.

@dain

dain Nov 22, 2018

Member

I dug into this a bit more. Uncaught exceptions are propagated down the ChannelInboundHandler path. This handler is the end of this path so this should work.

@@ -107,6 +107,10 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
}
log.error(cause);
if (ctx.channel().isActive()) {

This comment has been minimized.

@dain

dain Nov 9, 2018

Member

Do we need the if check? Why not unconditionally close the context.

This comment has been minimized.

@arhimondr

arhimondr Nov 12, 2018

Contributor

I took it from the io.netty.handler.ssl.SslHandler. I'm not sure why it was added there. Maybe just to safe some cycles on Future creation.

This comment has been minimized.

@dain

dain Nov 22, 2018

Member

Just call close unconditionally.

@arhimondr arhimondr force-pushed the arhimondr:check-if-pooled-connection-is-open branch from 8eb4d5d to d3ca370 Nov 12, 2018

@arhimondr

This comment has been minimized.

Contributor

arhimondr commented Nov 12, 2018

@dain @electrum comments addressed

@@ -211,6 +211,20 @@ private static void assertEchoService(EchoService service)
private static void assertThrowingService(ThrowingService service)
{
try {

This comment has been minimized.

@dain

dain Nov 22, 2018

Member

Did you mean to move this code?

This comment has been minimized.

@arhimondr

arhimondr Nov 22, 2018

Contributor

Yes. So if does more requests after it fails with TooLargeFrame. That's the way i caught the bug.

This comment has been minimized.

@electrum

electrum Dec 12, 2018

Member

Add a comment after this explaining

// make sure requests work after the server sends a frame that is too large
}
Future<Channel> future;

This comment has been minimized.

@dain

dain Nov 22, 2018

Member

no need for the extra blank line

}
catch (ExecutionException e) {
throw new RuntimeException(e);
cachedConnections.asMap().remove(key, future);

This comment has been minimized.

@dain

dain Nov 22, 2018

Member

add comment remove dead connection from cache

public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
{
// Don't log connection closed exceptions
if (isConnectionClosed(cause)) {

This comment has been minimized.

@dain

dain Nov 22, 2018

Member

Hey, can we test ctx to see if the connection is closed?

Just a thought, but maybe it is a better test for logging or not. @electrum what do you think?

This comment has been minimized.

@dain

dain Nov 22, 2018

Member

BTW, I debugged this and at this point the channel is still marked as open and active

@@ -107,6 +107,10 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
}
log.error(cause);
if (ctx.channel().isActive()) {

This comment has been minimized.

@dain

dain Nov 22, 2018

Member

Just call close unconditionally.

@@ -92,6 +98,17 @@ public void channelRead(ChannelHandlerContext context, Object message)
context.fireChannelRead(message);
}
@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)

This comment has been minimized.

@dain

dain Nov 22, 2018

Member

I dug into this a bit more. Uncaught exceptions are propagated down the ChannelInboundHandler path. This handler is the end of this path so this should work.

@dain

Some minor stuff, but otherwise looks good to me. @electrum did you want to review?

@arhimondr arhimondr force-pushed the arhimondr:check-if-pooled-connection-is-open branch from d3ca370 to 7e0da29 Nov 22, 2018

@arhimondr

This comment has been minimized.

Contributor

arhimondr commented Nov 22, 2018

@dain updated

public class ThriftServerHandler
extends ChannelDuplexHandler
{
private static final Logger log = Logger.get(ThriftServerHandler.class);
private static final Pattern CONNECTION_CLOSED_EXCEPTION_MESSAGE_PATTERN = Pattern.compile(

This comment has been minimized.

@electrum

electrum Dec 12, 2018

Member

Maybe shorten to CONNECTION_CLOSED_MESSAGE

private boolean isConnectionClosed(Throwable t)
{
if (t instanceof IOException) {
String message = t.getMessage();

This comment has been minimized.

@electrum

electrum Dec 12, 2018

Member

Could shorten this to

return CONNECTION_CLOSED_MESSAGE.matcher(nullToEmpty(t.getMessage()).matches();
@@ -107,6 +107,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
}
log.error(cause);
ctx.close();

This comment has been minimized.

@electrum

electrum Dec 12, 2018

Member

I'm thinking we should close this unconditionally, just in case the regex has a false-positive

@@ -211,6 +211,20 @@ private static void assertEchoService(EchoService service)
private static void assertThrowingService(ThrowingService service)
{
try {

This comment has been minimized.

@electrum

electrum Dec 12, 2018

Member

Add a comment after this explaining

// make sure requests work after the server sends a frame that is too large
}
// check if connection is failed or closed
Channel channel = future.getNow();

This comment has been minimized.

@electrum

electrum Dec 12, 2018

Member

Inline this variable?

arhimondr added some commits Nov 9, 2018

Check if pooled connection is still open
Closing a netty channel is an asynchronous operation. Before the connection
is closed, a new request may come in and grab a connection that is about to be
closed by the previous request.

@arhimondr arhimondr force-pushed the arhimondr:check-if-pooled-connection-is-open branch from 7e0da29 to 3b261d3 Dec 12, 2018

Close connection in case of unexpected exception in Drift server
Otherwise the clients may be hanging forever, as the connection may be
no longer in a consistent state

@arhimondr arhimondr force-pushed the arhimondr:check-if-pooled-connection-is-open branch from 3b261d3 to 2504e57 Dec 12, 2018

@electrum electrum merged commit 035d946 into airlift:master Dec 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment