Self-Cancelling Task Inhibits other Instances

Question

I am trying to make a subclass of Task that cancels itself and waits when it is disposed. While unit testing I got strange failing test cases. In the end it boiled down to repetition; I could even run the same test multiple times and get a pass only in the first run.

Here's the code for my AutoCancelTask class (sorry for the rather long code... I tried to make a minimum functional example that you can copy-paste...):

using System;
using System.Threading.Tasks;
using System.Threading;

namespace Tests
{
    /// <summary>
    /// A Task that is cancelled and waited for, when disposed.
    /// </summary>
    public class AutoCancelTask : Task
    {
        private readonly CancellationTokenSource _cts;

        private AutoCancelTask(Action<CancellationToken> action, CancellationTokenSource cts, TaskCreationOptions options)
            : base(() => action(cts.Token), cts.Token, options)
        {
            _cts = cts;
        }

        /// <summary>
        /// Wrap a cancellable Action in a Task that is automatically cancelled and waited for when disposed.
        /// </summary>
        /// <param name="action">The Action to wrap.</param>
        /// <param name="options">The TaskCreationOptions to use.</param>
        /// <returns>The started task.</returns>
        public static Task Run(Action<CancellationToken> action, TaskCreationOptions options = TaskCreationOptions.None)
        {
            Task t = new AutoCancelTask(action, new CancellationTokenSource(), options);
            t.Start();
            return t;
        }

        protected override void Dispose(bool disposing)
        {
            if (disposing)
            {
                try
                {
                    _cts.Cancel();
                }
                catch (ObjectDisposedException)
                {
                    // nothing to do in this case
                }
                try
                {
                    Wait();
                }
                catch (ObjectDisposedException) { }
                catch (AggregateException ex)
                {
                    if (1 < ex.InnerExceptions.Count || false == (ex.InnerExceptions[0] is TaskCanceledException))
                    {
                        // rethrow if the aggregate does not contain a single TaskCancelledException
                        throw ex;
                    }
                }
                _cts.Dispose();
            }

            base.Dispose(disposing);
        }
    }
}

And here's my test class

using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using System.Threading;

namespace Tests
{
    public class TestDataWrapper
    {
        internal bool? IsCancelled { get; private set; } = null;
        internal void Action(CancellationToken cancel)
        {
            IsCancelled = Task.Delay(100, cancel).ContinueWith(t => t.IsCanceled).Result;
        }
    }

    [TestClass()]
    public class Tests
    {
        public static IEnumerable<object[]> Datas { get; } =
            Enumerable.Range(0, 5).Select(_ => new object[] { new TestDataWrapper() });

        [DataTestMethod]
        [DynamicData(nameof(Datas))]
        public void AutoCancelTask_Cancels(TestDataWrapper data)
        {
            using (AutoCancelTask.Run(data.Action)) { }
            Assert.AreEqual(true, data.IsCancelled);
        }
    }
}

Obviously, I'm expecting the test case to pass. However, only the first execution passes, while the other ones fail with the message Assert.AreEqual failed. Expected:<True>. Actual:<(null)>..

Interestingly, execution time varies too: Run 1: ~30ms Run 2: ~300ms Run 4-5: <1ms

As the result is null and not false, I suspect the issue to be somewhere in the task being cancelled before the action is even started. However, this puzzles me, as I don't use anything static or otherwise shared between the test runs.

To make sure I understand the ContinueWith expression correctly, I tried replacing the Action method with the following:

internal void Action(CancellationToken cancel)
{
    Task t = Task.Delay(100, cancel);
    try
    {
        t.Wait();
    }
    catch
    {
        // ignore
    }
    IsCancelled = t.IsCanceled;
    t.Dispose();
}

That yields the same results.

I then tried to spam my class with debug output, and suddenly I got all test runs passed. It also works when I add a delay in the Dispose method exactly here:

if (disposing)
{
    try
    {
        Delay(1).Wait(); // <-- this fixes it
        _cts.Cancel();
    }
    catch (ObjectDisposedException)
    {
        // nothing to do in this case
    }
}

Which hardens my suspicion that a subsequent task can not start too soon after another has cancelled...

I'd rather avoid that delay there, as it seems not correct to me. Can you shed some light for me what is happening here?

Answer 1

Your tasks are most likely cancelled before they get the chance to run. Starting a task will put it on a queue for later execution. In this test you are cancelling the task immediately after creation.

So when the task is picked up for execution, its cancellation token is checked, and if cancelled, it is just set directly to cancelled status. So data.Action is never run, and you flag never set. Adding a delay will give the framework enough time to start executing the task.

Note that this approach seem to be highly susceptible to deadlocks. If you use this AutoCancelTask on the UI thread, and the task needs to dispatch anything to the same UI thread, you will get a deadlock. The task cannot complete since it waits for the UI thread, and the UI thread cannot continue, since it is waiting for the task.

The correct approach will depend on what you are trying to do. I would try to avoid inheriting from Task, since I think it will cause confusion. For example, It is fairly common to not bother disposing tasks, so if you add behavior to dispose you risk bugs if someone else are not aware of your special class. If you are on c# 8+ you can use IAsyncDisposable to allow awaiting in dispose methods and that might help in some use cases.