11

I have code that creates a cancellation token

public partial class CardsTabViewModel : BaseViewModel
{
   public CancellationTokenSource cts;

public async Task OnAppearing()
{
   cts = new CancellationTokenSource(); // << runs as part of OnAppearing()

Code that uses it:

await GetCards(cts.Token);


public async Task GetCards(CancellationToken ct)
{
    while (!ct.IsCancellationRequested)
    {
        App.viewablePhrases = App.DB.GetViewablePhrases(Settings.Mode, Settings.Pts);
        await CheckAvailability();
    }
}

and code that later cancels this Cancellation Token if the user moves away from the screen where the code above is running:

public void OnDisappearing()
{
   cts.Cancel();

Regarding cancellation, is this the correct way to cancel the token when it's being used in a Task?

In particular I checked this question:

Use of IsCancellationRequested property?

and it's making me think that I am not doing the cancel the correct way or perhaps in a way that can cause an exception.

Also, in this case after I have cancelled then should I be doing a cts.Dispose()?

Alan2
  • 23,493
  • 79
  • 256
  • 450
  • Normally, use the Cancel method to communicate a request for cancellation and then use Dispose method to release the memory. You could check the sample in link.https://learn.microsoft.com/en-us/dotnet/api/system.threading.cancellationtokensource.cancel?view=netframework-4.8 – Wendy Zang - MSFT Dec 04 '19 at 08:22

3 Answers3

4

In general I see a fair use of Cancel Token in your code, but according the Task Async Pattern your code could be not cancelling immediately.

while (!ct.IsCancellationRequested)
{
   App.viewablePhrases = App.DB.GetViewablePhrases(Settings.Mode, Settings.Pts);
   await CheckAvailability();   //Your Code could be blocked here, unable to cancel
}

For responding right away, the blocking code also should be cancelled

await CheckAvailability(ct);   //Your blocking code in the loop also should be stoped

It is up to you if you must Dispose, if there are many memory resources been reserved in the interrupted code, you should do it.

Fidel Orozco
  • 998
  • 11
  • 19
  • 1
    And indeed this would also apply to the call to GetViewablePhrases - ideally this would be an async call as well and take in a cancellation token as an option. – Paddy Dec 10 '19 at 13:44
2

CancellationTokenSource.Cancel() is a valid way to start cancellation.

Polling ct.IsCancellationRequested avoids throwing OperationCanceledException. Because its polling, it requires an iteration of the loop to complete before it will respond to the cancellation request.

If GetViewablePhrases() and CheckAvailability() can be modified to accept a CancellationToken, this may make cancellation faster to respond, at the cost of having OperationCanceledException thrown.

"should I be doing a cts.Dispose()?" is not that straightforward...

"Always dispose IDisposables ASAP"

Is more of a guideline than a rule. Task itself is disposable, yet hardly ever directly disposed in code.

There are cases (when WaitHandle or cancellation callback handlers are used) where disposing cts would free a resource / remove a GC root which otherwise would only be freed by a Finalizer. These don't apply to your code as it stands but may in future.

Adding a call to Dispose after cancelling would guarantee that these resources are freed promptly in future versions of the code.

However, you'd have to either wait for the code that uses cts to finish before calling dispose, or modify the code to deal with ObjectDisposedException from use of cts (or its token) after disposal.

Peter Wishart
  • 11,600
  • 1
  • 26
  • 45
  • "hook up OnDisappearing to dispose cts" Seems like a very bad idea, because, it's still in use inside another task. Particularly if someone does later change the design (modify the subtasks to accept a `CancellationToken` parameter), you could be disposing the `WaitHandle` while another thread is actively waiting on it :( – Ben Voigt Dec 06 '19 at 16:57
  • 1
    In particular, because you made the claim that "cancel performs the same cleanup as dispose", it would be pointless to call `Dispose` from `OnDisappearing`. – Ben Voigt Dec 06 '19 at 16:57
  • Whoops, I missed that the code in the answer already calls `Cancel`... – Peter Wishart Dec 06 '19 at 21:03
  • Have deleted the claim about cancel doing the same cleanup (which I'd read elsewhere on s.o.), as far as I can tell the only cleanup `Cancel` does is the internal timer (if used). – Peter Wishart Dec 10 '19 at 15:00
1

I would recommend you to take a look on one of the .net classes to fully understand how to handle wait methods with CanncelationToken, I picked up SeamaphoreSlim.cs

    public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken)
    {
        CheckDispose();

        // Validate input
        if (millisecondsTimeout < -1)
        {
            throw new ArgumentOutOfRangeException(
                "totalMilliSeconds", millisecondsTimeout, GetResourceString("SemaphoreSlim_Wait_TimeoutWrong"));
        }

        cancellationToken.ThrowIfCancellationRequested();

        uint startTime = 0;
        if (millisecondsTimeout != Timeout.Infinite && millisecondsTimeout > 0)
        {
            startTime = TimeoutHelper.GetTime();
        }

        bool waitSuccessful = false;
        Task<bool> asyncWaitTask = null;
        bool lockTaken = false;

        //Register for cancellation outside of the main lock.
        //NOTE: Register/deregister inside the lock can deadlock as different lock acquisition orders could
        //      occur for (1)this.m_lockObj and (2)cts.internalLock
        CancellationTokenRegistration cancellationTokenRegistration = cancellationToken.InternalRegisterWithoutEC(s_cancellationTokenCanceledEventHandler, this);
        try
        {
            // Perf: first spin wait for the count to be positive, but only up to the first planned yield.
            //       This additional amount of spinwaiting in addition
            //       to Monitor.Enter()’s spinwaiting has shown measurable perf gains in test scenarios.
            //
            SpinWait spin = new SpinWait();
            while (m_currentCount == 0 && !spin.NextSpinWillYield)
            {
                spin.SpinOnce();
            }
            // entering the lock and incrementing waiters must not suffer a thread-abort, else we cannot
            // clean up m_waitCount correctly, which may lead to deadlock due to non-woken waiters.
            try { }
            finally
            {
                Monitor.Enter(m_lockObj, ref lockTaken);
                if (lockTaken)
                {
                    m_waitCount++;
                }
            }

            // If there are any async waiters, for fairness we'll get in line behind
            // then by translating our synchronous wait into an asynchronous one that we 
            // then block on (once we've released the lock).
            if (m_asyncHead != null)
            {
                Contract.Assert(m_asyncTail != null, "tail should not be null if head isn't");
                asyncWaitTask = WaitAsync(millisecondsTimeout, cancellationToken);
            }
                // There are no async waiters, so we can proceed with normal synchronous waiting.
            else
            {
                // If the count > 0 we are good to move on.
                // If not, then wait if we were given allowed some wait duration

                OperationCanceledException oce = null;

                if (m_currentCount == 0)
                {
                    if (millisecondsTimeout == 0)
                    {
                        return false;
                    }

                    // Prepare for the main wait...
                    // wait until the count become greater than zero or the timeout is expired
                    try
                    {
                        waitSuccessful = WaitUntilCountOrTimeout(millisecondsTimeout, startTime, cancellationToken);
                    }
                    catch (OperationCanceledException e) { oce = e; }
                }

                // Now try to acquire.  We prioritize acquisition over cancellation/timeout so that we don't
                // lose any counts when there are asynchronous waiters in the mix.  Asynchronous waiters
                // defer to synchronous waiters in priority, which means that if it's possible an asynchronous
                // waiter didn't get released because a synchronous waiter was present, we need to ensure
                // that synchronous waiter succeeds so that they have a chance to release.
                Contract.Assert(!waitSuccessful || m_currentCount > 0, 
                    "If the wait was successful, there should be count available.");
                if (m_currentCount > 0)
                {
                    waitSuccessful = true;
                    m_currentCount--;
                }
                else if (oce != null)
                {
                    throw oce;
                }

                // Exposing wait handle which is lazily initialized if needed
                if (m_waitHandle != null && m_currentCount == 0)
                {
                    m_waitHandle.Reset();
                }
            }
        }
        finally
        {
            // Release the lock
            if (lockTaken)
            {
                m_waitCount--;
                Monitor.Exit(m_lockObj);
            }

            // Unregister the cancellation callback.
            cancellationTokenRegistration.Dispose();
        }

        // If we had to fall back to asynchronous waiting, block on it
        // here now that we've released the lock, and return its
        // result when available.  Otherwise, this was a synchronous
        // wait, and whether we successfully acquired the semaphore is
        // stored in waitSuccessful.

        return (asyncWaitTask != null) ? asyncWaitTask.GetAwaiter().GetResult() : waitSuccessful;
    }

You can also view the whole class here, https://referencesource.microsoft.com/#mscorlib/system/threading/SemaphoreSlim.cs,6095d9030263f169

Muhab
  • 367
  • 1
  • 2
  • 13