Improve CancellationException
#3039
Merged
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: 1. `H2ClientParentConnectionContext.StacklessCancellationException` was reused by `SpliceFlatStreamToMetaSingle`, which created a false impression in logs that this exception is related to HTTP/2. 2. There is not much value in a stacktrace for `CancellationException` in `BeforeFinallyHttpOperator`. Modifications: 1. Make `H2ClientParentConnectionContext.StacklessCancellationException` a top level class. 2. Create a `StacklessCancellationException` in http-utils module for `BeforeFinallyHttpOperator`. 3. Add exception message in `DefaultBlockingIterableProcessor`. Result: Easier for users to reason about received `CancellationException`.
idelpivnitskiy
commented
Aug 9, 2024
|
|
||
| import java.util.concurrent.CancellationException; | ||
|
|
||
| final class StacklessCancellationException extends CancellationException { |
There was a problem hiding this comment.
I was not sure if it's worth making it public in a module like concurrent-internal and created a copy for now. Can move if someone prefers.
There was a problem hiding this comment.
Feels a bit confusing to have the same exception in two different places, especially since http-netty depends on utils?
There was a problem hiding this comment.
Actually, I changed my mind. It's worth capturing the full stack-trace if we capture the Subscriber's stack-trace instead of the netty transport stack-trace. Updated PR
Scottmitch
approved these changes
Aug 13, 2024
daschl
approved these changes
Aug 13, 2024
bryce-anderson
approved these changes
Aug 13, 2024
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:
H2ClientParentConnectionContext.StacklessCancellationExceptionwas reused bySpliceFlatStreamToMetaSingle, which created a false impression in logs that this exception is related to HTTP/2.CancellationExceptioninBeforeFinallyHttpOperatorinstead of a netty stack-trace, it will help to debug paths that subscribe to the payload post cancel.Modifications:
H2ClientParentConnectionContext.StacklessCancellationExceptiona top-level class.CancellationExceptionwithPublisher.defer(...)inBeforeFinallyHttpOperator.DefaultBlockingIterableProcessor.Result:
Easier for users to reason about received
CancellationException.