Skip to content
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

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

geoffkizer
Copy link
Contributor

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.

@ghost
Copy link

ghost commented Jul 8, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

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.

Author: geoffkizer
Assignees: -
Labels:

area-System.Net

Milestone: -

@geoffkizer geoffkizer requested review from wfurt, ManickaP and CarnaViire and removed request for wfurt July 8, 2021 16:06
@ghost
Copy link

ghost commented Jul 8, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

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.

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@geoffkizer geoffkizer added this to the 6.0.0 milestone Jul 8, 2021
Copy link
Member

@wfurt wfurt left a 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));
Copy link
Member

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

Copy link
Contributor Author

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.

@wfurt wfurt merged commit d9f1ade into dotnet:main Jul 8, 2021
@geoffkizer geoffkizer deleted the canwrite branch July 8, 2021 21:08
@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants