shutdownOutput() for SSL connections prematurely closes the Channel
#1502
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: Before `RequestResponseCloseHandler` invokes `shutdownOutput()`, it forces `SslHandler` to close `SSLEngine` and send `close_notify` to the remote peer. Because `SSLEngine` is closed, `SslHandler` will generate `SslCloseCompletionEvent` on the next read regardless of receiving the `close_notify` alert from the remote peer or not. This premature event is handled incorrectly by the `DefaultNettyConnection` and forces the channel closure, breaking graceful closure handling on the server-side. Modifications: - Move handling of `SslCloseCompletionEvent` to the `CloseHandler` that is aware of the current state; - `RequestResponseCloseHandler` now ignores `SslCloseCompletionEvent` if it knows that the outbound was shutdown. It will wait for the `FIN` from remote peer to finish graceful closure; - Enhance `RequestResponseCloseHandlerTest` to validate new behavior; Result: 1. `shutdownOutput()` does not break graceful closure. 2. apple#1192 use-case `[4: protocol=HTTP_1 initiateClosureFromClient=false useUds=false viaProxy=true]` is not flaky anymore.
tkountis
reviewed
Apr 20, 2021
|
@Scottmitch can you take a look please? |
|
Discussed with @Scottmitch offline |
bondolo
pushed a commit
to bondolo/servicetalk
that referenced
this pull request
May 7, 2021
…apple#1502) Motivation: Before `RequestResponseCloseHandler` invokes `shutdownOutput()`, it forces `SslHandler` to close `SSLEngine` and send `close_notify` to the remote peer. Because `SSLEngine` is closed, `SslHandler` will generate `SslCloseCompletionEvent` on the next read regardless of receiving the `close_notify` alert from the remote peer or not. This premature event is handled incorrectly by the `DefaultNettyConnection` and forces the channel closure, breaking graceful closure handling on the server-side. Modifications: - Move handling of `SslCloseCompletionEvent` to the `CloseHandler` that is aware of the current state; - `RequestResponseCloseHandler` now ignores `SslCloseCompletionEvent` if it knows that the outbound was shutdown. It will wait for the `FIN` from remote peer to finish graceful closure; - Enhance `RequestResponseCloseHandlerTest` to validate new behavior; Result: 1. `shutdownOutput()` does not break graceful closure. 2. apple#1192 use-case `[4: protocol=HTTP_1 initiateClosureFromClient=false useUds=false viaProxy=true]` is not flaky anymore.
hbelmiro
pushed a commit
to hbelmiro/servicetalk
that referenced
this pull request
May 23, 2021
…apple#1502) Motivation: Before `RequestResponseCloseHandler` invokes `shutdownOutput()`, it forces `SslHandler` to close `SSLEngine` and send `close_notify` to the remote peer. Because `SSLEngine` is closed, `SslHandler` will generate `SslCloseCompletionEvent` on the next read regardless of receiving the `close_notify` alert from the remote peer or not. This premature event is handled incorrectly by the `DefaultNettyConnection` and forces the channel closure, breaking graceful closure handling on the server-side. Modifications: - Move handling of `SslCloseCompletionEvent` to the `CloseHandler` that is aware of the current state; - `RequestResponseCloseHandler` now ignores `SslCloseCompletionEvent` if it knows that the outbound was shutdown. It will wait for the `FIN` from remote peer to finish graceful closure; - Enhance `RequestResponseCloseHandlerTest` to validate new behavior; Result: 1. `shutdownOutput()` does not break graceful closure. 2. apple#1192 use-case `[4: protocol=HTTP_1 initiateClosureFromClient=false useUds=false viaProxy=true]` is not flaky anymore.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Before
RequestResponseCloseHandlerinvokesshutdownOutput(), it forcesSslHandlerto closeSSLEngineand sendclose_notifyto the remote peer.Because
SSLEngineis closed,SslHandlerwill generateSslCloseCompletionEventon the next read regardless of receiving theclose_notifyalert from the remote peer or not. This premature event ishandled incorrectly by the
DefaultNettyConnectionand forces the channelclosure, breaking graceful closure and
connection: closehandling on theserver-side.
Modifications:
SslCloseCompletionEventto theCloseHandlerthat isaware of the current state;
RequestResponseCloseHandlernow ignoresSslCloseCompletionEventif itknows that the outbound was shutdown. It will wait for the
FINfromremote peer to finish graceful closure;
RequestResponseCloseHandlerTestto validate new behavior;NonPipelinedCloseHandler;Result:
shutdownOutput()does not break graceful closure.[4: protocol=HTTP_1 initiateClosureFromClient=false useUds=false viaProxy=true]is not flaky anymore.