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

[QUIC] QuicStream.ReadAsync against an aborted connection returns End Of Stream #32050

Closed
scalablecory opened this issue Feb 10, 2020 · 5 comments · Fixed by #52776
Closed

[QUIC] QuicStream.ReadAsync against an aborted connection returns End Of Stream #32050

scalablecory opened this issue Feb 10, 2020 · 5 comments · Fixed by #52776
Assignees
Labels
area-System.Net.Quic bug tenet-reliability Reliability/stability related issue (stress, load problems, etc.)
Projects
Milestone

Comments

@scalablecory
Copy link
Contributor

ReadAsync() will return 0, which indicates end of stream, when a connection was aborted. It should be throwing QuicConnectionAbortedException.

@scalablecory scalablecory added this to the 5.0 milestone Feb 10, 2020
@scalablecory scalablecory added this to To Do in HTTP/3 via automation Feb 10, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Feb 10, 2020
@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Feb 10, 2020
@scalablecory scalablecory moved this from To Do (Low Priority) to To Do (High Priority) in HTTP/3 Feb 10, 2020
@jkotalik jkotalik moved this from To Do (High Priority) to In Progress in HTTP/3 Feb 19, 2020
@jkotalik
Copy link
Contributor

@nibanks, when closing a connection in quic with a non-zero error code, does that propagate up to streams at all? Right now it looks like in msquic, an event occurs on the connection with an error code, but that doesn't propagate up to the streams; streams just close.

@jkotalik jkotalik moved this from In Progress to To Do (High Priority) in HTTP/3 Feb 19, 2020
@nibanks
Copy link

nibanks commented Feb 19, 2020

@jkotalik nope. Streams and connections have separate error code spaces. If you shutdown the connection with an error, the peer streams just get shutdown complete events, and the connection gets the peer shutdown with the error code.

@jkotalik
Copy link
Contributor

Hm, interesting. So if the peer shuts down a connection, are streams gracefully or abortively shut down? To me, if we just get shutdown/ shutdown complete events, it seems graceful even though we are "aborting" the connection.

@nibanks
Copy link

nibanks commented Feb 19, 2020

As far as QUIC is concerned, there is no such thing as "aborting" a connection. The app shuts it down with an error code. There are no "good" or "bad" error codes. When you shut down a connection with currently opened streams, the streams are implicitly shutdown, but since no app-specific error code was given to shut down the stream by the peer, none is delivered to the app. They are just implicitly shut down, and the corresponding events are delivered to the app.

@karelz karelz added the tenet-reliability Reliability/stability related issue (stress, load problems, etc.) label Feb 20, 2020
@karelz karelz changed the title QuicStream.ReadAsync against an aborted connection returns End Of Stream [QUIC] QuicStream.ReadAsync against an aborted connection returns End Of Stream Mar 11, 2020
@scalablecory scalablecory modified the milestones: 5.0.0, 6.0.0 Aug 11, 2020
@ManickaP
Copy link
Member

ManickaP commented Mar 4, 2021

Meeting notes:

  • Msquic is event-based, instead of calling Read we get an event that more data is available. We should also get an event about connection being closed. If we see that connection is closed, we should throw an exception from our Read instead of returning 0.
  • We also want to differentiate upon us closing vs peer closing, we should propagate error code in case of peer closing.
  • The only event that doesn't have error code is Shutdown Complete, but it is a result of our shutdown.
  • We decided for now to let the user track their own error codes and not round-tripping them back (in e.g. ObjectDisposedException) but we may add that later

@CarnaViire CarnaViire self-assigned this Apr 22, 2021
@CarnaViire CarnaViire moved this from To Do (High Priority) to In Progress in HTTP/3 Apr 22, 2021
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels May 12, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 14, 2021
HTTP/3 automation moved this from In Progress to Done May 26, 2021
CarnaViire added a commit that referenced this issue May 26, 2021
This leverages newly added flag to SHUTDOWN_COMPLETED event, which means that's not a user-initiated stream shutdown, but a connection abort.

Fixes #32050
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 26, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Quic bug tenet-reliability Reliability/stability related issue (stress, load problems, etc.)
Projects
No open projects
HTTP/3
  
Done
8 participants