Cancellable "fire and forget" task. Is it bad practice? Is my implementation correct?

1.2k views Asked by At

Background: I am developing an app that is essentially a cached file explorer for a remote server. When a user clicks on a directory it will show them its subdirectories from a local copy of the directory tree. It then also starts a "fire and forget" task to retrieve changes to directory in view from the server and update the cache and what the user is being displayed.

private CancellationTokenSource _cts;
private SemaphoreSlim _myTaskBlocker = new SemaphoreSlim(1,1);

public void CancelMyTask()
{
    _cts?.Cancel();
}

public async Task FireAndForgetWithCancel()
{
    await _myTaskBlocker.WaitAsync();
    _cts = new CancellationTokenSource();

    try
    {
        //Some potentially long running code.
        token.ThrowIfCancellationRequested();
    }
    catch(OperationCancelledException){}
    finally
    {        
        _cts.dispose();
        _cts = null;
        _myTaskBlocker.Release();
    }
}

EDIT 1: Is this okay to do? The SemaphoreSlim is sort of acting like a lock on _cts so I shouldn't need to lock it before making changes right?

EDIT 2: So I've come the conclusion that this is a bad idea and can't really be accomplished in the manner I had initially hoping for.

My solution is for the ViewModel to send a request and listen for an update event.

  public class Model
    {
        // Thread safe observable collection with AddRange.
        public ObservableCollectionEx<file> Files { get; }

        // A request queue.
        public ActionBlock<request> Requests { get; }
    }

    public class ViewModel
    {
        public ObservableCollectionEx<file> FilesTheUserSees { get; }

        public ViewModel()
        {
            Model.Files.CollectionChanged += FileCollectionChanged;
        }

        public async Task UserInstigatedEvent()
        {
            // Do some stuff to FilesTheUserSees here.

            // Request the model to check for updates. Blocks only as long as it takes to send the message.
            await Model.Requests.SendAsync(new request());
        }

        public void FileCollectionChanged(object sender, CollectionChangedEventArgs e)
        {
            // Check to see if there are any new files in Files.
            // If there are new files that match the current requirements add them to FilesTheUserSees.
        }
    }

Some issues to note is that there is now a reliance on ObservableCollectionsEx to be thread safe, however this is an accomplishable goal and easier to debug even if it has some down sides.

1

There are 1 answers

5
Sefe On

This is not a good use of the cancellation process with a CancellationToken. Your code has several issues that you wouldn't have with a correct use of cancellation tokens:

  1. CancelMyTask and FireAndForgetWithCancel are interdependent, because they are internally connected with a CancellationTokenSource that FireAndForgetWithCancel creates and destroys, but that CancelMyTask uses. You have only a small window when CancelMyTask will work and you have no direct indicator when cancelling would work.
  2. If you call FireAndForgetWithCancel multiple times, CancelMyTask will only cancel the last task, while the other tasks will continue.
  3. Since the FireAndForgetWithCancel creates its own CancellationTokenSource and keeps the CancellationToken for itself, no other consumer can use the token, for example to trigger its own cancellation or combine it with other tokens.
  4. In FireAndForgetWithCancel you catch the OperationCancelledException instead of letting it fall through. That means the task can never be in a cancelled state.

The correct usage is to accept the cancellation token as a parameter and let the caller handle the cancellation, as it is suggested by separating CancellationTokenSource and CancellationToken:

private SemaphoreSlim _myTaskBlocker = new SemaphoreSlim(1,1);

public async Task FireAndForgetWithCancel(CancellationToken cancellationToken)
{
    await _myTaskBlocker.WaitAsync();

    try
    {
        //Some potentially long running code.
        cancellationToken.ThrowIfCancellationRequested();
    }
    finally
    {        
        _myTaskBlocker.Release();
    }
}

The caller will either create a CancellationTokenSource to get a cancellation token or it will pass through an existing one. That will depend entirely on the caller and is best left to it.