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] Finalize QUIC dispose model #32142

Closed
scalablecory opened this issue Feb 11, 2020 · 7 comments
Closed

[QUIC] Finalize QUIC dispose model #32142

scalablecory opened this issue Feb 11, 2020 · 7 comments
Labels
area-System.Net.Quic enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@scalablecory
Copy link
Contributor

scalablecory commented Feb 11, 2020

Socket currently has a behavior of throwing SocketError.OperationAborted for outstanding operations when Dispose() is called.

We need to decide how Dispose() will trigger outstanding operations:

  • QuicListener -- outstanding AcceptConnection() calls.
  • QuicConnection -- outstanding Connect(), AcceptStream(), Close() calls. Plus any already open streams.
  • QuicStream -- outstanding Read(), Write(), Shutdown(), Abort() calls.

The current model throws an OperationAbortedException, mimicing the Socket API; We should look at how consistent we want to be here, and if we want to adjust for usability/perf reasons, such as having AcceptConnection() or having AcceptStream() return null rather than throwing.

We should also consider how this interacts with a user-initiated connection/stream abort, and with a server-initiated connection/stream abort. In sockets, a variety of error codes can be thrown for these, one of them being SocketError.OperationAborted.

@scalablecory scalablecory added this to the 5.0 milestone Feb 11, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Feb 11, 2020
@scalablecory scalablecory removed the untriaged New issue has not been triaged by the area owner label Feb 11, 2020
@karelz karelz added the enhancement Product code improvement that does NOT require public API changes/additions label Feb 20, 2020
@karelz karelz changed the title Finalize QUIC dispose model [QUIC] Finalize QUIC dispose model Mar 11, 2020
@GSPP
Copy link

GSPP commented Jun 12, 2020

What would this do if Dispose is called right before an operation is started? Would this throw ObjectDisposedException? If yes, this would cause an inherent race condition. For example, a caller of Socket.Accept cannot know whether he will receive SocketError.OperationAborted or an ODE. That makes the SocketError.OperationAborted not useful for handling this condition.

This is a thorny issue for Socket and it seems to apply just as much to QUIC. There has been some extensive discussion which might be useful to take note of: https://github.com/dotnet/corefx/issues/5749

Having an orderly was for shutting down is quite important as essentially any application must shut down eventually. This issue affects many users of Socket and QUIC.

@scalablecory scalablecory modified the milestones: 5.0.0, 6.0.0 Aug 11, 2020
@geoffkizer
Copy link
Contributor

I think from a usability point of view, returning null from AcceptConnection or AcceptStream when a local close has happened is the best behavior, even though it's different than the behavior of Socket.Accept.

AcceptStream can also fail because of a remote close, either graceful or abortive; I'm less sure what should happen in this case. I think you can make an argument that graceful remote close should result in null from AcceptStream. Abortive remote close probably shouldn't.

@geoffkizer
Copy link
Contributor

What would this do if Dispose is called right before an operation is started? Would this throw ObjectDisposedException? If yes, this would cause an inherent race condition.

Agreed, this is a good issue.

@geoffkizer
Copy link
Contributor

geoffkizer commented Nov 17, 2020

I think QuicStream issues are covered under #756.

Let's use this issue specifically for QuicConnection and QuicListener. (Or maybe we should open issues for each...)

Edit: I do think the AcceptConnection and AcceptStream issues are very similar; we should be consistent here. I think the only other issue then, is Dispose during Connect. Am I missing anything here?

@ManickaP
Copy link
Member

ManickaP commented Mar 4, 2021

Meeting notes:

  • Throw ObjectDisposedException in both scenarios: Read/etc called before Dispose, Read/etc called after Dispose. This will save us a race condition.
  • We decided not to add any error code to the exception for now.

@ManickaP
Copy link
Member

ManickaP commented Jul 15, 2021

Triage: we should investigate for 6.0 whether we're not hanging in case of a disposal and whether we're propagating a correct exception outside of the calls in flight.

Some of this is already fixed.

@ManickaP ManickaP modified the milestones: 6.0.0, 7.0.0 Aug 10, 2021
@ManickaP
Copy link
Member

Closing as this is covered by #67560 #68902 and #70669

@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Quic enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

7 participants