Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ig-sinicyn opened this issue Apr 21, 2020 · 8 comments · May be fixed by #46
Open

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

ig-sinicyn opened this issue Apr 21, 2020 · 8 comments · May be fixed by #46

Comments

@ig-sinicyn
Copy link

Motivation:
We start Gofer task client from .net core hosted service.
We use the BackgroundService.ExecuteAsync() method that expects that underlying code will exit on cancellation. We've imitate cancellation support with

cancellation.Register(()=>taskClient.CancelListen())

but I think it will be good to have such feature out of the box.

@brthor
Copy link
Owner

brthor commented Jul 11, 2020

Thanks for the feature request @ig-sinicyn .

Can you provide an example of how you expect the code should look if the API was changed to accommodate this use case?

@ig-sinicyn
Copy link
Author

something like

public async Task Listen(CancellationToken cancellation = default) { ... }

public CancellationTokenSource Start(CancellationToken cancellation = defaul)
{
   ...
   /// Now loop can be canceled via cancellation arg or via CancelListen() method.
   ListenCancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellation)
   ...
}

I think

@brthor
Copy link
Owner

brthor commented Jul 18, 2020

I don't see any reason why an overload couldn't be added that takes a cancellation token. Personally I have not used the pattern much but I understand it's widely spread throughout .NET.

Are you interested in making a PR for this feature @ig-sinicyn ?

@ig-sinicyn
Copy link
Author

ig-sinicyn commented Jul 19, 2020

Will do.
Personally, I do not have any good reasons for preferring optional parameter over a new overload, so I'll follow your suggestion:)

UPD done, #46

@brthor
Copy link
Owner

brthor commented Jul 29, 2020

The scope of this issue (and associated PR #46 ) is expanding to encompass Tasks being able to receive a CancellationToken tied to the TaskClient so that when the token is Canceled, tasks can perform their own shutdown.

@brthor
Copy link
Owner

brthor commented Jul 29, 2020

First implementation plugs in the CancellationToken from the TaskClient to the Task arguments using a special case on any arguments of type CancellationToken during serialization and invocation.

@brthor
Copy link
Owner

brthor commented Jul 29, 2020

Proposal copied from #46

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()
  1. 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.

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

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

@brthor
Copy link
Owner

brthor commented Jul 29, 2020

Copied from #46

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

@brthor brthor linked a pull request Jul 29, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants