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

[API Proposal]: CancellationTokenRegistration.OnUnregisteredWithoutInvoke(Action) #60843

Closed
timcassell opened this issue Oct 25, 2021 · 4 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading untriaged New issue has not been triaged by the area owner

Comments

@timcassell
Copy link

timcassell commented Oct 25, 2021

I started this as a discussion, but was invited to open an issue instead.

Background and motivation

I want to be able to have a custom object live for the lifetime of a cancellation registration without regard for any async operations. For example, I have a custom token that wraps around the System.Threading.CancellationToken and clean itself up if/when it is known that the cancelation callback will never be invoked (the source is disposed without calling Cancel, or the new TryReset is called).

API Proposal

namespace System.Threading
{
    public struct CancellationTokenRegistration
    {
        public void OnUnregisteredWithoutInvoke(Action callback);
    }
}

API Usage

public class MyCustomToken
{
    private CancellationTokenRegistration registration;
    
    public class MyCustomToken(CancellationToken token)
    {
        // Either OnCanceled will be invoked, _or_ OnUnregisteredWithoutInvoke will be invoked, not both.
        registration = token.Register(OnCanceled);
        registration.OnUnregisteredWithoutInvoke(OnUnregisteredWithoutInvoke);
    }
    
    private void OnCanceled()
    {
        DoCustomWork();
        Cleanup();
    }
    
    private void DoCustomWork()
    {
        // Do my custom thing.
    }
    
    private void OnUnregisteredWithoutInvoke()
    {
        Cleanup();
    }
    
    private void Cleanup()
    {
        // Custom Cleanup
        registration.Dispose();
    }
}

Alternative Designs

Change the signature of CancellationToken.Register to accept 2 callbacks, one for the regular callback, one for the unregistered.

Risks

No response

@timcassell timcassell added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 25, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Oct 25, 2021
@dotnet-issue-labeler
Copy link

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

@ghost
Copy link

ghost commented Oct 26, 2021

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

Issue Details

I started this as a discussion, but was invited to open an issue instead.

Background and motivation

I want to be able to have a custom object live for the lifetime of a cancellation registration without regard for any async operations. For example, I have a custom token that wraps around the System.Threading.CancellationToken and clean itself up if/when it is known that the cancelation callback will never be invoked (the source is disposed without calling Cancel, or the new TryReset is called).

API Proposal

namespace System.Threading
{
    public struct CancellationTokenRegistration
    {
        public void OnUnregisteredWithoutInvoke(Action callback);
    }
}

API Usage

public class MyCustomToken
{
    private CancellationTokenRegistration registration;
    
    public class MyCustomToken(CancellationToken token)
    {
        // Either OnCanceled will be invoked, _or_ OnUnregisteredWithoutInvoke will be invoked, not both.
        registration = token.Register(OnCanceled);
        registration.OnUnregisteredWithoutInvoke(OnUnregisteredWithoutInvoke);
    }
    
    private void OnCanceled()
    {
        DoCustomWork();
        Cleanup();
    }
    
    private void DoCustomWork()
    {
        // Do my custom thing.
    }
    
    private void OnUnregisteredWithoutInvoke()
    {
        Cleanup();
    }
    
    private void Cleanup()
    {
        // Custom Cleanup
        registration.Dispose();
    }
}

Alternative Designs

Change the signature of CancellationToken.Register to accept 2 callbacks, one for the regular callback, one for the unregistered.

Risks

No response

Author: timcassell
Assignees: -
Labels:

api-suggestion, area-System.Threading, untriaged

Milestone: -

@stephentoub
Copy link
Member

Thanks for the suggestion.

You can implement something like this yourself on top of the existing APIs, e.g.

public static class Extensions
{
    public static void OnUnregisteredWithoutInvoke(this CancellationToken cancellationToken, Action onCanceled, Action onDropped) =>
        cancellationToken.Register(new Wrapper(onCanceled, onDropped).Invoke);

    private sealed class Wrapper
    {
        private readonly Action _onCanceled;
        private readonly Action _onDropped;

        public Wrapper(Action onCanceled, Action onDropped)
        {
            _onCanceled = onCanceled;
            _onDropped = onDropped;
        }

        public void Invoke()
        {
            GC.SuppressFinalize(this);
            _onCanceled();
        }

        ~Wrapper() => _onDropped?.Invoke();
    }
}

and that will accommodate not just explicit Dispose or TryReset, but also someone simply dropping the CancellationTokenSource, which is relatively common.

I understand your use case, but I don't think it's common enough to warrant such an addition to CancellationToken. We would either end up implementing it essentially as I outlined above, at which point you could do so with whatever policy you wanted to imbue it with, or we would need to implement it in a non-pay-for-play manner, meaning all use of CancellationToken{Source} would get at least a little more expensive even if this functionality wasn't being used.

@timcassell
Copy link
Author

@stephentoub Ah, I hadn't even thought of using a finalizer for that! Thanks, I will use that. I was also a little worried that this wouldn't be implemented due to making the general case slightly more expensive, but it's good to know that it's still possible without doing that.

Since this is possible today with no changes, and it's such a niche use-case, I'll close this.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

3 participants