rational for fireExceptionCaught when an IOException will close the connection anyways

[Scottmitch]

To avoid letting exceptions flow through to the end of the pipeline and go unhanded one approach is to insert an inbound channel handler to the end of the pipeline which simply closes the connection if any unexpected exceptions are caught. In the case of the codec-http2 there is some logic which must be done when a channel is closed (send GO_AWAY and graceful shutdown). codec-http2 attempts to distinguish if the GO_AWAY should be attempted based upon the state of the channel, and will not attempt a GO_AWAY if the channel is not active [1].
Currently if while reading we catch an exception we call fireExceptionCaught and then close the channel [2]. When this happens the last channel inbound handler catches the unexpected exception and calls ctx.close(). codec-http2's close method is now called, but the state of the channel is still active and thus the GO_AWAY logic executed, but the send fails and logs a warning.

The question is what is the correct way to handle this situation?

• Should the channels still call fireExceptionCaught in cases where the connection will be closed?
• Is the current behavior acceptable, but the last channel inbound handler should have some logic to know when it is "OK" to call close (for example if the exception instanceof IOException ... this feels risky)
• Should codec-http2 (and other codecs) use some other indicator, or keep state based on exception caught to know the "real" channel heath?

[1] https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L414
[2] https://github.com/netty/netty/blob/4.1/transport/src/main/java/io/netty/channel/nio/AbstractNioByteChannel.java#L87

[Scottmitch]

@trustin @normanmaurer @nmittler @ejona86 - WDYT?

[ejona86]

Should the channels still call fireExceptionCaught in cases where the connection will be closed?

For gRPC we need to know why the channel was closed. Even with the current state, it is very easy to lose the shutdown reason and only have ClosedChannelException, which is useless. If you remove the fireExceptionCaught, then there won't be any semi-centralized place for us to record why the channel shut down.

Just failing the futures doesn't work well because there are so many places writes are performed and it is inefficient.

Is the current behavior acceptable

I'm fairly okay with the current behavior, except for the logspam. I'd rather see no processing of the future used with goaway or it log at the debug level instead of error/warn. Granted, we've not been running with some of the recent changes, so maybe this is no longer a problem.

In general I don't care if GOAWAY fails. The only time I do is if Http2ConnectionHandler or similar has a bug triggering it, which hasn't been the case yet.

Should codec-http2 (and other codecs) use some other indicator, or keep state based on exception caught to know the "real" channel heath?

I don't know what indicator that would be. Keeping state could maybe work, although I do think it may have a good number of problems, like exceptionCaught doesn't mean the connection will be closed since it could possibly be handled gracefully.

[Scottmitch]

Thanks for the feedback @ejona86.

Agreed I think keeping the current fireExceptionCaught behavior makes sense.

I'd rather see no processing of the future used with goaway or it log at the debug level instead of error/warn

We still need to process the future to close the channel in the happy path case. However reducing the log level to debug seems like a good approach in this situation. Since I broke this let me submit a PR.