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

QuicStream.WritesClosed continuation ran before the Quic reset frame is sent? #79911

Closed
bentoi opened this issue Dec 22, 2022 · 2 comments · Fixed by #90253
Closed

QuicStream.WritesClosed continuation ran before the Quic reset frame is sent? #79911

bentoi opened this issue Dec 22, 2022 · 2 comments · Fixed by #90253
Assignees
Milestone

Comments

@bentoi
Copy link

bentoi commented Dec 22, 2022

Kind of related to #79818 but for QuicStream.WritesClosed this time.

When aborting a stream writing side, it looks like the WritesClosed continuation is ran before the reset frame is actually sent to the peer. If the continuation disposes of the stream, the peer ReadAsync sometime completes successfully.

It's also actually not clear to me why DisposeAsync doesn't abort the stream with the default error code.

I've attached a test case. Here's the test case failure:

Running test...
Running test...
Running test...
Running test...
Running test...
Unhandled exception. System.InvalidOperationException: didn't expect ReadAsync to succeed with 0 bytes!
   at Program.Main(String[] args) in /home/vagrant/workspace/quicabortwrite/Program.cs:line 60
   at Program.Main(String[] args) in /home/vagrant/workspace/quicabortwrite/Program.cs:line 64
   at Program.Main(String[] args) in /home/vagrant/workspace/quicabortwrite/Program.cs:line 64
   at Program.Main(String[] args) in /home/vagrant/workspace/quicabortwrite/Program.cs:line 43
   at Program.<Main>(String[] args)

quicabortwrite.zip

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 22, 2022
@ghost
Copy link

ghost commented Dec 22, 2022

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

Issue Details

Kind of related to #79818 but for QuicStream.WritesClosed this time.

When aborting a stream writing side, it looks like the WritesClosed continuation is ran before the reset frame is actually sent to the peer. If the continuation disposes of the stream, the peer ReadAsync sometime completes successfully.

It's also actually not clear to me why DisposeAsync doesn't abort the stream with the default error code.

I've attached a test case. Here's the test case failure:

Running test...
Running test...
Running test...
Running test...
Running test...
Unhandled exception. System.InvalidOperationException: didn't expect ReadAsync to succeed with 0 bytes!
   at Program.Main(String[] args) in /home/vagrant/workspace/quicabortwrite/Program.cs:line 60
   at Program.Main(String[] args) in /home/vagrant/workspace/quicabortwrite/Program.cs:line 64
   at Program.Main(String[] args) in /home/vagrant/workspace/quicabortwrite/Program.cs:line 64
   at Program.Main(String[] args) in /home/vagrant/workspace/quicabortwrite/Program.cs:line 43
   at Program.<Main>(String[] args)

quicabortwrite.zip

Author: bentoi
Assignees: -
Labels:

untriaged, area-System.Net.Quic

Milestone: -

@ManickaP
Copy link
Member

ManickaP commented Jan 2, 2023

Yes, the order of setting the Task is before aborting on the wire. What should not happen is that DisposeAsync in this case closes gracefully the writing side, we should fix that.
As for why DisposeAsync closes gracefully the writing side in the default case. It's for when QuicStream is used as Stream, the base class, which knows nothing about CompleteWrites. We want to keep the "happy-path" of using (stream) { read; write; } to behave the same as other types of stream, i.e. to close gracefully.

Triage: we should fix the DisposeAsync behavior in 8.0. We might discuss more the timing of when the Writes/ReadsClosed tasks are finished in regards to the other operations.

@ManickaP ManickaP added bug and removed untriaged New issue has not been triaged by the area owner labels Jan 2, 2023
@ManickaP ManickaP added this to the 8.0.0 milestone Jan 2, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 9, 2023
@karelz karelz modified the milestones: 8.0.0, 9.0.0 Aug 14, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 13, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants