Server control flow should close connection on DecoderException
#2419
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
Scottmitch
reviewed
Nov 18, 2022
| final Completable exchange = requestSingle.flatMapCompletable(rawRequest -> { | ||
| final Completable exchange = requestSingle.whenOnError(t -> { | ||
| // If an error happened before we could start writing, we need to close the channel. | ||
| nettyChannel().close(); |
There was a problem hiding this comment.
when we close under the hood it can be difficult for users to understand who initiated closure. Consider either:
- moving this close operation into
ErrorLoggingHttpSubscriberafter it logs the reason - adding a comment that
ErrorLoggingHttpSubscriberwill log the exception if required.
There was a problem hiding this comment.
Good point, I've moved closure to ErrorLoggingHttpSubscriber
|
lgtm after the above is addressed. |
…h the client-side (apple#2367)" (apple#2423)" This reverts commit da109dd.
idelpivnitskiy
force-pushed
the
flow-control-fix
branch
from
6dc8078 to
dd4874b
Compare
|
I've reverted back server-side control flow as the first commit and applied a change of this PR as the second commit. Will use "Rebase and merge" |
Motivation: apple#2367 changed server-side control flow. After that, `connection.write` doesn't start before we received a request meta-data. As the result, all errors that happen during parsing of the meta-data do not reach `WriteStreamSubscriber` and can leave a connection in undefined state, when we logged an error but didn't clean up the channel state. Modifications: - Close the channel inside `ErrorLoggingHttpSubscriber` if it was not closed already. It covers a case when an error occurs on the `Single` of request meta-data; - Add tests to verify the expected behavior; Result: Server channel is closed when an early request error happens, client doesn't hang forever.
idelpivnitskiy
force-pushed
the
flow-control-fix
branch
from
dd4874b to
3e4195a
Compare
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Dec 17, 2022
…erException` (apple#2419)"" This reverts commit de8721e.
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Dec 17, 2022
…erException` (apple#2419)"" This reverts commit de8721e.
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Dec 17, 2022
…erException` (apple#2419)"" This reverts commit de8721e.
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:
#2367 changed server-side control flow. After that,
connection.writedoesn't start before we received a request meta-data. As the result, all errors that happen during parsing of the meta-data do not reachWriteStreamSubscriberand can leave a connection in undefined state, when we logged an error but didn't clean up the channel state.Modifications:
Singleof request meta-data;Result:
Channel is closed when an early request error happens, client doesn't hang forever.