-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 read abort handling and revert CanRead/CanWrite to previous behavior #55341
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsWe are currently throwing OperationCancelledException when the peer aborts our write. We should be throwing QuicStreamAbortedException with the appropriate error code. Also fix mock provider to handle write abort and add test. Also, #54798 changed CanRead/CanWrite behavior to return false when completed or aborted, which broke some tests that rely on CanRead/CanWrite to distinguish between unidirectional and bidirectional streams. Revert the behavior of CanRead/CanWrite to previous behavior -- that is, only change these to false when the object is disposed.
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsWe are currently throwing OperationCancelledException when the peer aborts our write. We should be throwing QuicStreamAbortedException with the appropriate error code. Also fix mock provider to handle write abort and add test. Also, #54798 changed CanRead/CanWrite behavior to return false when completed or aborted, which broke some tests that rely on CanRead/CanWrite to distinguish between unidirectional and bidirectional streams. Revert the behavior of CanRead/CanWrite to previous behavior -- that is, only change these to false when the object is disposed.
|
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
{ | ||
await using QuicStream stream = await connection.AcceptStreamAsync(); | ||
|
||
QuicStreamAbortedException ex = await Assert.ThrowsAsync<QuicStreamAbortedException>(() => WriteForever(stream)); |
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.
We discussed previously that it's good to add .WaitAsync(someReasonableTimeout)
to WriteForever
-like operations, it would still fail the assertion, but we won't have a hanging test
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.
RunClientServer has a timeout, so the test won't hang forever.
We are currently throwing OperationCancelledException when the peer aborts our write. We should be throwing QuicStreamAbortedException with the appropriate error code. Also fix mock provider to handle write abort and add test.
Also, #54798 changed CanRead/CanWrite behavior to return false when completed or aborted, which broke some tests that rely on CanRead/CanWrite to distinguish between unidirectional and bidirectional streams. Revert the behavior of CanRead/CanWrite to previous behavior -- that is, only change these to false when the object is disposed.