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() #48492

Closed
halter73 opened this issue Feb 19, 2021 · 9 comments · Fixed by #50346
Closed

Add CancellationTokenSource.TryReset() #48492

halter73 opened this issue Feb 19, 2021 · 9 comments · Fixed by #50346
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Feb 19, 2021

Background and Motivation

When a library or framework exposes a CancellationToken that usually does not get canceled (e.g. HttpContext.RequestAborted) they often still need to dispose the backing CancellationTokenSource (CTS) after an operation completes rather than reusing it in order to account for callers that might never dispose their registrations.

To use the RequestAborted example, Kestrel disposes the backing CTS after every request where the RequestAborted token is accessed. If Kestrel attempted to reuse the CTS backing the RequestAborted token for future requests in order to reduce allocations, it would risk leaking any undisposed registrations and triggering the undisposed registrations when unrelated requests are aborted.

Another scenario where people want to be able to "reset" a CTS is after calling CancelAfter(). This can already be achieved by calling CancelAfter(Timeout.Infinite), but that's not necessarily obvious unless you read the docs. TryReset() is something that would immediately make sense in this scenario when looking at intellisense completions.

Another benefit is that it's immediately obvious if resetting failed. If you try resetting a timeout with CancelAfter(), you have to check after the call to verify their was no cancellation before or during the call causing CancelAfter() to no-op. This is demonstrated in the second HttpClient usage example.

Proposed API

namespace System.Threading
{
    public class CancellationTokenSource : IDisposable
    {
+        // Returns false if the CTS has already been canceled
+        public bool TryReset();    
    }
}

Usage Examples

Simplified RequestAborted example.

private readonly CancellationTokenSource _abortCts = new CancellationTokenSource();

// Imagine this could be called in the background because a client disconnect or some timeout.
void AbortConnection()
{
    _abortCts.Cancel();
}

async Task ProcessRequestsAsync()
{
   while (_abortCts.TryReset())
   {
      var httpContext = await ParseRequest(_abortCts.Token);
      httpContext.RequestAborted = _abortCts.Token;
      await Middleware.InvokeAsync(httpContext);
   }
}

HttpClient request

var cts = new CancellationTokenSource();
var httpClient = new HttpClient();

while (true)
{
    cts.CancelAfter(TimeSpan.FromSeconds(10));
    var response = await httpClient.GetAsync("http://example.org/healthcheck", cts.Token);
    response.EnsureSuccessStatusCode();
    
    if (!cts.TryReset())
    {
        cts = new CancellationTokenSource();
    }
}

Today, without TryReset(), you could achieve similar functionality like so:

var cts = new CancellationTokenSource();
var httpClient = new HttpClient();

while (true)
{
    cts.CancelAfter(TimeSpan.FromSeconds(10));

    if (cts.Token.IsCancellationRequested)
    {
        cts = new CancellationTokenSource(TimeSpan.FromSeconds(10));
    }

    // ...

So in the case of timeouts, the proposed reset APIs don't offer any entirely new functionality, but it exposes it in a much more obvious way.

Alternative Designs

Since calling CancelAfter(Timeout.Infinite) should already reset any timeouts, we could consider an API that just clears registrations. You could call both CancelAfter(Timeout.Infinite) and TryClearRegistrations() to get the full TryReset() behavior mentioned above.

namespace System.Threading
{
    public class CancellationTokenSource : IDisposable {
+        public bool TryClearRegistrations();    
    }
}

Risks

People might assume they can reset a CTS that has already been canceled. We should clearly document this isn't the case. Hopefully the Try in the name will get people thinking about the cases in which it won't work.

Code that references a stale token and registers after the backing CTS has been reset can run into the same problems those who don't dispose their registrations today face when the CTS is reused. That might not sound bad, but this API will likely encourage more reuse of CTS objects. I do think that not disposing registrations is more common than using stale tokens though.

@halter73 halter73 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 19, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Feb 19, 2021
@ghost
Copy link

ghost commented Feb 19, 2021

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

Issue Details

Background and Motivation

When a library or framework exposes a CancellationToken that usually does not get canceled (e.g. HttpContext.RequestAborted) they often still need to dispose the backing CancellationTokenSource (CTS) after an operation completes rather than reusing it in order to account for callers that might never dispose their registrations.

To use the RequestAborted example, Kestrel disposes the backing CTS after every request where the RequestAborted token is accessed. If Kestrel attempted to reuse the CTS backing the RequestAborted token for future requests in order to reduce allocations, it would risk leaking any undisposed registrations and triggering the undisposed registrations when unrelated requests are aborted.

Another scenario where people want to be able to "reset" a CTS is after calling CancelAfter(). This can already be achieved by calling CancelAfter(Timeout.Infinite), but that's not necessarily obvious unless you read the docs. TryReset() is something that would immediately make sense in this scenario when looking at intellisense completions.

Another benefit is that it's immediately obvious if resetting failed. If you try resetting a timeout with CancelAfter(), you have to check after the call to verify their was no cancellation before or during the call causing CancelAfter() to no-op. This is demonstrated in the second HttpClient usage example.

Proposed API

namespace System.Threading
{
    public class CancellationTokenSource : IDisposable {
+        // Returns false if the CTS has already been canceled
+        public bool TryReset();    
    }
}

Usage Examples

Simplified RequestAborted example.

private readonly CancellationTokenSource _abortCts = new CancellationTokenSource();

// Imagine this could be called in the background because a client disconnect or some timeout.
void AbortConnection()
{
    _abortCts.Cancel();
}

async Task ProcessRequestsAsync()
{
   while (_abortCts.TryReset())
   {
      var httpContext = await ParseRequest(_abortCts.Token);
      httpContext.RequestAborted = _abortCts.Token;
      await Middleware.Invoke(httpContext);
   }
}

HttpClient request

var cts = new CancellationTokenSource();
var httpClient = new HttpClient();

while (true)
{
    cts.CancelAfter(TimeSpan.FromSeconds(10));
    var response = await httpClient.GetAsync("http://example.org/healthcheck", cts.Token);
    response.EnsureSuccessStatusCode();
    
    if (!cts.TryReset())
    {
        cts = new CancellationTokenSource();
    }
}

Today, without TryReset(), you could achieve similar functionality like so:

var cts = new CancellationTokenSource();
var httpClient = new HttpClient();

while (true)
{
    cts.CancelAfter(TimeSpan.FromSeconds(10));

    if (cts.Token.IsCancellationRequested)
    {
        cts = new CancellationTokenSource(TimeSpan.FromSeconds(10));
    }

    // ...

So in the case of timeouts, the proposed reset APIs don't offer any entirely new functionality, but it exposes it in a much more obvious way.

Alternative Designs

Since calling CancelAfter(Timeout.Infinite) should already reset any timeouts, we could consider an API that just clears registrations. You could call both CancelAfter(Timeout.Infinite) and TryClearRegistrations() to get the full TryReset() behavior mentioned above.

namespace System.Threading
{
    public class CancellationTokenSource : IDisposable {
+        public bool TryClearRegistrations();    
    }
}

Risks

People might assume they can reset a CTS that has already been canceled. We should clearly document this isn't the case. Hopefully the Try in the name will get people thinking about the cases in which it won't work.

Code that references a stale token and registers after the backing CTS has been reset can run into the same problems those who don't dispose their registrations today face when the CTS is reused. That might not sound bad, but this API will likely encourage more reuse of CTS objects. I do think that not disposing registrations is more common than using stale token though.

Author: halter73
Assignees: -
Labels:

api-suggestion, area-System.Net.Http, untriaged

Milestone: -

@halter73 halter73 changed the title Add ConnectionTokenSource.Reset() Add ConnectionTokenSource.TryReset() Feb 19, 2021
@YairHalberstadt
Copy link
Contributor

People might assume they can reset a CTS that has already been canceled.

Why can't they?

@halter73
Copy link
Member Author

Resetting a canceled CTS would uncancel associated CancellationTokens. That's a surprising behavior we'd like to avoid.

@YairHalberstadt
Copy link
Contributor

I'm assuming that you can only use Reset if you know (or can reasonably assume) that there are no cancellation tokens in the wild. Otherwise reusing the CTS for something else would cancel them at the wrong time, plus removing registrations would also be surprising.

@stephentoub
Copy link
Member

stephentoub commented Feb 19, 2021

I'm assuming that you can only use Reset if you know (or can reasonably assume) that there are no cancellation tokens in the wild.

Yes. TryReset really wouldn't be doing much to "reset" the CTS, other than stop any associated timer and clear out registrations that really shouldn't have been there in the first place if the code was properly constructed, and from that perspective the name is a little strange, but I don't have a better one in mind. CancellationToken is just a struct-wrapper for a CancellationTokenSource, so any CT previously handed out will still be referencing this CTS, and any usage of that CT after TryReset would still work exactly as it previously did. So, the user of this method would simply be saying "I plan to reuse this CTS because the operation with which it was previously associated is now complete; as such, I'm going to clear out registrations because I'm not going to ever cancel them, anyway". If either the owner calling the TryReset was wrong, or if code still tried to use the "old" CT handed out prior to the TryReset call, that's a bug that code needs to fix.

I think this is reasonable. It's a bit dangerous if used poorly, but much, much less dangerous than a CancellationToken transitioning from canceled to not-canceled.

I think we'd also want to be a little fuzzy about what a false return value means, if for no other reason than future proofing. If true, feel free to reuse the token as it wasn't canceled. If false, it was either canceled or for some reason we didn't feel comfortable suggesting you can reuse it. The 99% case will be that it returns true, as cancellation is the minority case.

@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 19, 2021
@stephentoub stephentoub added this to the 6.0.0 milestone Feb 19, 2021
@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Feb 19, 2021
@davidfowl
Copy link
Member

Will this clear out the free node list as well? I was hoping that would be reusable. That could basically result in 0 allocations even for re-registers across uses. It does mean that the free list can grow longer without a way to clear it or detect that is has grown though.

@stephentoub
Copy link
Member

Will this clear out the free node list as well?

Implementation detail, but functionally it needn't, so I'd start with "no". When I prototyped it, I explicitly did the opposite, transferring any stale registrations to the free list.

@davidfowl
Copy link
Member

That's what I wanted to hear 😁

@GrabYourPitchforks GrabYourPitchforks changed the title Add ConnectionTokenSource.TryReset() Add CancellationTokenSource.TryReset() Mar 16, 2021
@davidfowl davidfowl added the blocking Marks issues that we want to fast track in order to unblock other important work label Mar 26, 2021
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Mar 26, 2021
@terrajobst
Copy link
Member

terrajobst commented Mar 26, 2021

Video

  • Looks good as proposed
namespace System.Threading
{
    public partial class CancellationTokenSource
    {
        public bool TryReset();    
    }
}

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 29, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 30, 2021
@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.
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants