-
Notifications
You must be signed in to change notification settings - Fork 902
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
Fix delayed notification of session creation failure and log GOAWAY frames #138
Conversation
} | ||
|
||
@Override | ||
public void channelInactive(ChannelHandlerContext ctx) throws Exception { | ||
failPendingResponses(ClosedSessionException.INSTANCE); | ||
ctx.fireChannelInactive(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok not to call this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because it's the last handler in the pipeline.
04033c6
to
5d041e5
Compare
@Override | ||
public void onGoAwayReceived(int lastStreamId, long errorCode, ByteBuf debugData) { | ||
onGoAway("Received", lastStreamId, errorCode, debugData); | ||
ch.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't find any documentation that says we need to do this, Http2ConnectionAdapter itself has an empty method. If this is needed, can you add a comment to better explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a GOAWAY is received, DefaultHttp2Connection.goAwayReceived(...)
is invoked. It will notify the listeners and then close all active streams. It does not attempt to close a connection, and it seems like no known Http2Connection.Listener.onGoAwayReceived()
implementation closes the connection either. It just waits for the client to close the connection, which doesn't sound like a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments. Also, changed not to call close() if we sent GOAWAY, because it will trigger the disconnection.
…rames Motivation: When a connection is closed before session protocol negotiation succeeds or fails, the Promise of session creation is not notified immediately but only after the session creation timeout occurs. As a result, the notification of session creation due to an unexpected disconnection can take up to as long as connection timeout (3.2 seconds by default) Also, we need to log more: - the information about GOAWAY frames we sent and received - the current session protocol when an unexpected exception occurred Modifications: - Try to reject the session creation promise when a connection has been closed. - Add Http2GoAwayListener and add it to both client and server connections so that we have more information about GOAWAY frames we send and receive. - Log the current session protocol when logging an unexpected exception - Log the pre-upgrade request and the removal of the upgrade stream to see if there's a case where the upgrade stream is removed before sending the first response - Miscellaneous: - Do not propagate IdleStateEvent in Http*IdleTimeoutHandler. Result: - Session creation failure is notified much sooner in the situation described above. - We will have more information about GOAWAY frames and unexpected behaviors related with session protocol negotiation
5d041e5
to
74e18db
Compare
Motivation:
When a connection is closed before session protocol negotiation succeeds
or fails, the Promise of session creation is not notified immediately
but only after the session creation timeout. As a result, the
notification of session creation due to an unexpected disconnection can
take up to as long as connection timeout (3.2 seconds by default)
Also, we need to log more:
Modifications:
closed.
connections so that we have more information about GOAWAY frames we
send and receive.
see if there's a case where the upgrade stream is removed before
sending the first response
Result:
described above.
behaviors related with session protocol negotiation