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

Add CancellationTokenSource.TryReset #50346

Merged
merged 2 commits into from
Mar 30, 2021
Merged

Conversation

stephentoub
Copy link
Member

Fixes #48492

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@GrabYourPitchforks
Copy link
Member

Trying to wrap my mind around what happens if we have a linked CTS (via CTS.CreateLinkedTokenSource). Let's say that we have two parent CT instances: parent1 and parent2, and we create a child CTS child:

CancellationToken parent1 = GetToken();
CancellationToken parent2 = GetToken();
CancellationTokenSource child = CancellationTokenSource.CreateLinkedTokenSource(parent1, parent2);

From reading through this code, it looks like if either parent1's CTS or parent2's CTS is reset, then that particular token tripping will no longer trigger the child being canceled, but the other token can still trigger a cancellation. That seems fine.

But what happens if somebody calls child.TryReset()? It clears any registrations tied directly to it, but it's still listening for cancellation notifications coming from its parents, correct?

What I'm trying to figure out is how the phrase "usage of TryReset concurrently with issuing cancellation is not thread-safe" squares with the fact that we might have a linked CTS. Since I assume most linked CTS instances don't "own" the parent and can't control when the parent triggers cancellation, it would be ill-advised to call TryReset on a CTS returned by the CreateLinkedTokenSource API?

@stephentoub
Copy link
Member Author

From reading through this code, it looks like if either parent1's CTS or parent2's CTS is reset, then that particular token tripping will no longer trigger the child being canceled, but the other token can still trigger a cancellation. That seems fine.

Correct. If you have:

var cts2 = CreateLinkedTokenSource(cts1.Token);

then this is basically the same as:

var cts2 = new CancellationTokenSource();
cts2._registrationToDispose = cts1.Token.Register(() => cts2.Cancel());

So if you call cts1.TryReset, it'll drop the registration to call cts2.Cancel, and if cts1 has cancellation requested, it won't forward that along to cts2.

But what happens if somebody calls child.TryReset()? It clears any registrations tied directly to it, but it's still listening for cancellation notifications coming from its parents, correct?

Correct

What I'm trying to figure out is how the phrase "usage of TryReset concurrently with issuing cancellation is not thread-safe" squares with the fact that we might have a linked CTS. Since I assume most linked CTS instances don't "own" the parent and can't control when the parent triggers cancellation, it would be ill-advised to call TryReset on a CTS returned by the CreateLinkedTokenSource API?

Right. It's no different than if you handed out a reference to your CTS to someone else that might call Cancel{After}. You're no longer the sole owner, and so using TryReset is problematic. You won't corrupt the instance, but you might return true even if the CTS is already canceled.

@GrabYourPitchforks
Copy link
Member

You won't corrupt the instance, but you might return true even if the CTS is already canceled.

That seems fine then. A race condition is much preferred to corruption. :)

@stephentoub
Copy link
Member Author

That seems fine then. A race condition is much preferred to corruption. :)

I'll augment the comment to clarify. Thanks.

@stephentoub stephentoub merged commit e830f10 into dotnet:main Mar 30, 2021
@stephentoub stephentoub deleted the ctsreset branch March 30, 2021 00:44
@davidfowl
Copy link
Member

It's time ❗

cc @BrennanConroy

@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 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.

Add CancellationTokenSource.TryReset()
3 participants