Add cancellation token support

[ig-sinicyn]

This PR

1. Adds cancellation token support for TaskClient Listen()/Start() methods.
2. 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]

up

[brthor]

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

[brthor]

@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]

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]

up

[brthor]

@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:

1. Create static method GetTaskCancellationToken on the TaskClient
2. 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()

3. 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.

4. 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.

5. 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]

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]

@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.

[ig-sinicyn]

@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]

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]

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 👍

[ig-sinicyn]

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]

up)

[ig-sinicyn]

up)

[ig-sinicyn]

+1 up