-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[QUIC] Cosmetic changes to Read pipeline #55591
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFollow-up for NITs and cosmetic changes from #55505
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFollow-up for NITs and cosmetic changes from #55505
|
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.
LGTM, thanks!
abortError = _state.ReadErrorCode; | ||
|
||
if (readState != ReadState.PendingRead && cancellationToken.IsCancellationRequested) | ||
// Failure scenario: pre-canceled token. Transition: any -> Aborted |
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.
Love the comments ❤️
_state.ReadState = ReadState.Aborted; | ||
canceledSynchronously = true; | ||
preCanceled = true; |
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.
Why don't directly return new OperationCanceledException(cancellationToken)
here and get rid of preCanceled
local var. We wouldn't even need to change the initialReadState
if I'm reading the code correctly.
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.
To do all the work on exceptions in one place and outside of the lock
Follow-up for NITs and cosmetic changes from #55505