Add cancellation token support #46

ig-sinicyn · 2020-07-19T11:12:38Z

This PR

Adds cancellation token support for TaskClient Listen()/Start() methods.
Adds cancellation support for enqueued tasks.
- I've added JsonSkipCancellationTokenConverter to ensure same logic will be applied even if the TaskInfo is created manually
- The InvokeMethod() method accepts CancellationToken arg and replaces target method cancellation arg with the passed token.

Tests included.

ig-sinicyn · 2020-07-19T11:16:48Z

Gofer.NET.Tests/GivenATaskClient.cs

+
+            var task = Task.Run(async () => await taskClient.Listen(cancellation.Token), CancellationToken.None);
+            await Task.Delay(waitTime, CancellationToken.None);
+            cancellation.Cancel();


Enqueued task (WaitForTaskClientCancellationAndWriteSemaphore) will be canceled at this moment.

ig-sinicyn · 2020-07-23T09:53:08Z

up

brthor · 2020-07-24T19:21:12Z

Thank you for including tests with your PR. I am starting to look over it.

Gofer.NET.Tests/GivenARedisTaskQueue.cs

brthor · 2020-07-24T19:29:36Z

Gofer.NET.Tests/GivenATaskClient.cs

@@ -28,37 +29,59 @@ public async Task ItContinuesListeningWhenATaskThrowsAnException()

            taskClient.CancelListen();
            await task;
-
+
+            TaskQueueTestFixture.EnsureSemaphore(semaphoreFile);


Looks like a bug in the test, good catch!

Gofer.NET.Tests/GivenATaskInfo.cs

brthor · 2020-07-24T19:33:26Z

Gofer.NET.Tests/GivenATaskInfo.cs

+            var taskInfo3b = GetTestTask(() => TestMethod3(new CancellationTokenSource().Token));
+
+            var taskInfo4a = GetTestTask(() => Console.WriteLine("hello"));
+            var taskInfo4b = GetTestTask(() => Console.WriteLine("hello world"));


👍 System assemblies do present an uncovered test case here.

Gofer.NET.Utils/ActionExtensionMethods.cs

Gofer.NET.Tests/TestQueueTestFixture.cs

Gofer.NET.Utils/ActionExtensionMethods.cs

brthor · 2020-07-24T19:41:27Z

Gofer.NET.Utils/JsonSkipCancellationTokenConverter.cs

+
+namespace Gofer.NET.Utils
+{
+    public class JsonSkipCancellationTokenConverter : JsonConverter


Unless you can provide a compelling reason why the cancellation token should be excluded from the serialization, this functionality should be removed altogether.

I've added cancellation support for enqueued tasks. Json.Net cannot serialize CancellationToken anyway so it was an easiest fix I've could imagine.

brthor · 2020-07-24T19:42:10Z

Gofer.NET.Utils/TaskInfo.cs

@@ -43,7 +44,7 @@ public bool IsEquivalent(TaskInfo otherTaskInfo)
            }

            for (var i=0; i<Args.Length; ++i) {
-                if (!Args[i].Equals(otherTaskInfo.Args[i]))
+                if (!Equals(Args[i], otherTaskInfo.Args[i]))


reason for this change?

There will be a NulIRefException if Args[i] is null. Actually, I've catched one and did the change to mitigate it.

brthor · 2020-07-24T19:45:00Z

@ig-sinicyn
I did not read your PR description fully before beginning my review. Before I continue further, let's discuss number 2 in your description.

Is the idea that upon canceling the cancellation token passed to TaskClient.Listen that the cancellation should be propagated to the workers?

ig-sinicyn · 2020-07-25T09:55:32Z

Is the idea that upon canceling the cancellation token passed to TaskClient.Listen that the cancellation should be propagated to the workers?

Yeah. I've started with adding cancellation to the client's Start()/Listen() methods. But then I've found that there is no way to notify workers about cancellation. So, I've added this feature, too.

ig-sinicyn

@brthor Thanks for the good review! I've made requested changes, let me know if there are more:)

…token to the IsEquivalent() method

ig-sinicyn · 2020-07-29T18:45:59Z

up

brthor · 2020-07-29T22:10:54Z

@ig-sinicyn

I let this roll over in my brain for a couple days.

So pretty much my interpretation of what you're doing is allowing users to create tasks that take a cancellation token as an argument and then plugging in a cancellation token tied to the particular TaskClient that begins running that task, so that if that token is cancelled, the task receives the signal and can tidy things up itself.

The intention is good, that the tasks get the cancellation signal and can handle their own stopping.

The implementation, whereby you plug in a value for the token based on an available CancellationToken type argument in the task makes me worried.

Firstly, it requires digging into the args, as I already commented on, and doing special tricks with them on serialization and at invocation time. In general, this is a behavior I'd like to stay away from as much as possible because it can cause unexpected behavior for users who haven't taken the time to familiarize themselves with this special behavior and naively use a task with a CancellationToken argument for whatever reason.

A broader form of the argument is that this special behavior will interfere with regular usage where no intention was presented by the user for a CancellationToken tied to the TaskClient.

So, if the functionality is good, but the implementation has some issues with user-experience, what is the ideal implementation? In my thinking the ideal implementation accommodates the user who wants a CancellationToken tied to the TaskClient but at the same time is only available to the user who specifically requests it.

So I make the following proposal to satisfy those requirements:

Create static method GetTaskCancellationToken on the TaskClient
When this method is called examine the call stack to find the TaskClient instance currently running Listen for the task.

Rough mockup:

--> TaskClient.GetTaskCancellationToken() --> [Task Method] --> TaskInfo.ExecuteTask() --> TaskClient.Listen()

If there is no calling instance of TaskClient in the stack, throw a specific exception, this method can only be used by tasks running inside a TaskClient Listen loop.
Return the CancellationToken associated with the current TaskClient.Listen() loop. Since the TaskClient currently enforces only one listen loop running at a time, this should be as simple as storing the cancellation token in a property when .Listen() is called.
If the currently running Listen loop had no cancellation token passed to it, return the cancellation token created by the TaskClient at the start of Listen, similar to the current implementation.

My preliminary research shows that this implementation is feasible, subject to some testing.

This implementation provides the user a way of obtaining a CancellationToken tied to the TaskClient with a clear gesture that specifies their intention rather than providing a convention that may interfere with regular usage without that intention.

Examining the call stack may seem a little overkill, but this method is likely to be used infrequently only at the start of a task, minimizing performance concerns. Also, assuming it works well when tested, it's likely to be fairly durable.

Please let me know your thoughts.

EDIT: See below comment on using AsyncLocal instead of examining the call stack.

brthor · 2020-07-29T22:33:21Z

TaskClient.cs

        {
            TaskQueue = taskQueue;
            OnError = onError;
            TaskScheduler = new TaskScheduler(TaskQueue);
            IsCanceled = false;
        }

-        public async Task Listen()
+        public Task Listen()


To clean up the mix of being able to pass in a cancellation token here and at the same time have a default cancellation token source I recommend the following:

Change the Start(CancellationToken cancellation) overload to Start(CancellationTokenSource cancellationTokenSource)

Inside of the Listen(CancellationToken cancellation) use CancellationTokenSource.CreateLinkedTokenSource(cancellation) to create a CancellationTokenSource to pass to Start()

Inside the Listen() method overload, do not call the other overload of Listen but instead duplicate the code and call the Start() overload with no parameters.

Inside the Start() overload with no parameters use new CancellationTokenSource() and use it to call the other overload of Start Start(CancellationToken cancellation)

Current implementation has a very low overhead so I'd prefer to keep it until there are some scenarios it does not fit nicely.

Official docs recommend to pass CancellationToken, not CancellationTokenSource. Linked cancellation token is a standard approach for supporting both external and internal cancellation. So there is nothing unusual and surprising for end-user code.

brthor · 2020-07-29T22:54:27Z

Upon a little more research it seems that examining the call stack and getting the actual calling object instance cannot easily be accomplished.

The AsyncLocal construct appears to be a much simpler approach that offers the same benefits.

https://docs.microsoft.com/en-us/dotnet/api/system.threading.asynclocal-1?view=netcore-3.1

In this case a private static field can be used to track the Local Cancellation Token (set during the Start method).

Then the GetTaskCancellationToken simply returns that private field.

ig-sinicyn · 2020-07-30T18:25:39Z

@brthor good catch and astonishing level of detail. Thanks for a great response 👍

I do agree with proposed changes, no objections here. Will push fix in a day or two.

…place them with a AsyncLocal<CancellationToken>();

ig-sinicyn · 2020-08-20T11:52:06Z

@brthor sorry for the nasty delay:( Had a hard month and literally no spare time at all.

I've made changes you've suggested. Code is much more cleaner now and area of changes was reduced a lot. Win-win:)

brthor · 2020-08-23T03:10:43Z

Gofer.NET.Tests/GivenATaskClient.cs

+            await Task.Delay(waitTime, CancellationToken.None);
+            cancellation.Cancel();
+            await Task.Delay(waitTime, CancellationToken.None);
+            await task;


If a bug is introduced, and the task fails to exit on cancellation will this introduce a hanging test?

brthor · 2020-08-23T03:22:35Z

Gofer.NET.Tests/TestQueueTestFixture.cs

+                throw new InvalidOperationException("This method must be called from a task client callback");
+            try
+            {
+                await Task.Delay(-1, token);


I guess the real potential for a test hang is here. Perhaps a polling loop with a timeout?

Nice catch! Fixed with a timeout parameter.

brthor · 2020-08-23T03:29:53Z

TaskClient.cs

-
+                    _listenCancellationContext.Value = token;
+
+                    var executionTimer = Stopwatch.StartNew();


brthor · 2020-08-23T03:32:22Z

No worries @ig-sinicyn . I think overall it's looking pretty good and the async local change does clean it up a lot!

My only concern left is the potential test hang, I think it's a pretty quick fix though.

I'm going to re-enable the CI on this so the tests will run.

brthor · 2020-08-23T03:45:38Z

Looks like the CI is running but not posting status updates.

Link is here: https://travis-ci.org/github/brthor/Gofer.NET

Tests are passing 👍

…phore + Fix TimeSpan and DateTimeOffset parameter passing

ig-sinicyn · 2020-08-23T13:11:26Z

If a bug is introduced, and the task fails to exit on cancellation will this introduce a hanging test?

Thanks for spotting, have missed it!

I've fixed the issue with timeout parameter. As it turned out TimeSpan and DateTimeOffset args were not deserialized properly so I've fixed args deserialzation as well:)

ig-sinicyn · 2020-08-28T13:01:37Z

up)

ig-sinicyn · 2020-09-07T05:55:36Z

up)

ig-sinicyn · 2020-10-09T18:07:00Z

+1 up

Add cancellation token support

5eda310

ig-sinicyn mentioned this pull request Jul 19, 2020

[FeatureRequest] Add overload with cancellation token to the Listen() method #44

Open

ig-sinicyn commented Jul 19, 2020

View reviewed changes