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] Abort after Dispose is no-op #57318

Merged
merged 1 commit into from
Aug 13, 2021

Conversation

CarnaViire
Copy link
Member

Allow calling Abort(Read|Write) after disposing. The only safe option to do so is to make them no-op in that case (see my comment in code).

Fixes #56683

@ghost
Copy link

ghost commented Aug 12, 2021

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

Issue Details

Allow calling Abort(Read|Write) after disposing. The only safe option to do so is to make them no-op in that case (see my comment in code).

Fixes #56683

Author: CarnaViire
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@ghost
Copy link

ghost commented Aug 12, 2021

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

Issue Details

Allow calling Abort(Read|Write) after disposing. The only safe option to do so is to make them no-op in that case (see my comment in code).

Fixes #56683

Author: CarnaViire
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had an idea, we don't have to act on that.
The change is good as it is, thanks!

{
// Dispose already triggered graceful shutdown
// It is unsafe to try to trigger abortive shutdown now, because final event arriving after Dispose releases SafeHandle
// so if it arrives after our check but before we call msquic, me might end up with access violation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it help if we send the abort only if the SendState was < Aborted? Few lines bellow we test the state, we could as well use similar pattern we do elsewhere with shouldAbort, set it to true only if we change the state and send the Abort only in such cases.

If this is something we could do, it might also help with the concern you raised in #57156, since it would provide the "locking" security via the SendState property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline that the suggestion is good, but not directly related to the change, so it should be done in a separate PR

@CarnaViire CarnaViire merged commit d67efc5 into dotnet:main Aug 13, 2021
@CarnaViire CarnaViire deleted the quic-allow-abort-after-dispose branch August 13, 2021 12:19
@karelz karelz added this to the 6.0.0 milestone Aug 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 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.

HTTP/3: Cancellation causing ObjectDisposedException
3 participants