Repository navigation
Add better support for cancellation tokens passing #133
Description
Activity
Implementation progress: #132. Earlier discussion (see @bartelink and @gusty's comments): #114.
Let me answer your questions from the other thread here, @bartelink:
People could cope with this, and it's obviously terse. It's also arguably a dual to the
do! fun ct -> ...construct in IcedTasks.Yes, that's similar, but not quite the same. This proposal is about setting the token. In IcedTasks, it is about getting the token. You need then provide your actual token when you run the (delayed) task.
Allowing people to access the token by writing such code could be much easier then having them passing the token manually through their own functions.
would you ever want to setCT more than once?
Task sequences are "read many". If a task sequence is passed on to several consumers, each could set their own cancellation token. Which in turn brings up another question. This should not be part of the
taskSeqstate, but of theIAsyncEnumeratorstate (that is: not the enumerable!). People may conceive it that way with thedo!syntax.Now, this is actually done correctly with the
do!approach. It will only assign the cancellation token once enumeration actually starts, after callingGetAsyncEnumerator(ct). It effectively overrides whatever is inct, so this is another concern (for manual iteration).Is there any way the CT can become an argument to the builder?
Yes, this is possible, but it doesn't solve the other issues I raised above, unfortunately. Also, you may not always want to pass the cancellation token at the moment of creation. I.e., it's not uncommon to consume a task sequence and wanting to control cancellation from the consumer side.
Thanks for the clarifications regarding my (naive) questions, and for thoroughly mapping out the overall requirements.|
I agree that
setCancellationToken/getCancellationTokenas proposed are good baseline APIs given the constraints being resolved and unfortunately can't see any cleaner solution.I haven't followed the repo in a few weeks, so am answering at a high level
- If a TaskSeq is eliminated to a primitive value (e.g. `int) an optional cancellation token parameter can be provided
- If a TaskSeq is converted to an Task an optional cancellation token parameter can be provided
- If a TaskSeq is converted to an Async (e.g. some
TaskSeq.lengthAsync) or CancellableTask/ColdTask then the TaskSeq should get its cancellation from that computation
For each function in the API accepting
TaskSeqas input this is an automatic process- Look at the outputs, and see if it implies a cancellation token (TaskSeq, Async, CancellableTask...). If so, use that
- If no output implies a cancellation token, then add an optional cancellation token as an input parameter.
For example, following these principles rigorously:
TaskSeq.length: TaskSeq<'T> * ?ct:CancellationToken -> intTaskSeq.lengthAsync: TaskSeq<'T> -> Async<int>should get its cancellation token from the AsyncTaskSeq.lengthAsTask: TaskSeq<'T> * ?ct:CancellationToken -> Task<int>TaskSeq.lengthAsCancellableTask: TaskSeq<'T> -> CancellableTask<int>TaskSeq.toList: TaskSeq<'T> * ?ct:CancellationToken -> 'T listTaskSeq.toListAsync: TaskSeq<'T> -> Async<'T list>TaskSeq.toListAsTask: TaskSeq<'T> * ?ct:CancellationToken -> Task<'T list>
Note that you'd have to a TaskSeq class containing static members to use the optional methods.
One case that's a little unclear to me is whether a result type of
Seqshould use the Dispose, e.g.TaskSeq.toSeqBlocking: TaskSeq<'T> -> 'T seq// blocks the current thread on each MoveNext, each uncancellable, Dispose of seq cancels.TaskSeq.toSeqOfAsync: TaskSeq<'T> -> Async<'T> seq// Dispose of seq enumerator cancels? Each individual MoveNext results in an Async using the cancellation token for that asyncTaskSeq.toSeqOfTasks: TaskSeq<'T> -> Task<'T> seq// Dispose of seq enumerator is the overall cancellation signal?
In AsyncSeq and Async this was resolved by adding overloads that take an optional cancellation token. Not ideal either.
In short, I believe this is the only correct/viable solution. Anything else is making cancellation impossible to reason about accurately and reliably, and you may as well not build it in to the CE.
@dsyme thanks for the high level insights (if you have the time, I’d like to discuss TaskEx as a new FsProjects lib, to harmonise the proliferation of task helpers in every task-related lib out there; I’ll ping you on Slack).
I kinda feel icky about leaving the module and using static members on the type, it feels non-idiomatic, but to allow normal composition and piping with optional params, that’s the only way afaik (and
Asyncdoes the same).This should not be part of the
taskSeqstate, but of theIAsyncEnumeratorstate (that is: not the enumerable!).All your comments above seem to apply to this (from OP). I think we’re in agreement. Once the
taskSeqis enumerated, a token should be passed (blocking andTaskresults) or passed through (Asyncresults).The only thing left out here is how to pass or get a token to a for loop in the
AsyncorTaskCE. But I’ve some idea how to do just that.In short, I believe this is the only correct/viable solution.
I’ve now come to the same conclusion, thanks!
The only thing left out here is how to pass or get a token to a for loop in the Async or Task CE. But I’ve some idea how to do just that.
For Async or CancellableTask you use
Async.CancellationToken/CancellableTask.cancellationTokenFor
Taskit isn't possible unfortunately - but you could have an overload of Task.For taking an IAsyncEnumerator I suppose?task { for x in taskSeq.GetEnumerator(ct) do ... }
I didn’t consider that, but yeah, that may be possible. A bit tricky, perhaps, as our
IAsyncEnumerableitself implementsIAsyncEnumeratorto avoid an unnecessary shadow copy. I’ll have to check, it’s probably no big deal, as I think I cast before returning, so unless it is recast, it shouldn’t matter.Just thinking out loud ;).
I never considered this before, but this approach appears to work quite well, albeit with a few caveats (/cc @dsyme):
type TaskSeq = static member isEmpty source : Task<bool> = Internal.isEmpty CancellationToken.None source static member isEmpty(token: CancellationToken) : (taskSeq<'T> -> Task<bool>) = Internal.isEmpty token // example of using it module Test = let f() = TaskSeq.empty |> TaskSeq.isEmpty CancellationToken.None let g() = TaskSeq.empty |> TaskSeq.isEmpty
Note that this is deliberately not the same as the tupled approach above. There are a few advantages for this approach:
- The cancellation token is in its natural position (left-most vs right-most in the tupled approach)
- The functions can be used in functional piping and composition styles, even with cancellation token (not possible with the tuple approach)
- It has a more natural FP feel
And some disadvantages:
- These are essentially single-argument overloaded functions, with different result types (a function vs a value)
- There's less tooltip-support for the function arguments, as the second and further arguments will be part of the returned function and can therefore not be named or xml-documented
- Potential extra overhead if the compiler doesn't auto-inline these returned functions
I will add the main disadvantage of this ad-hoc overloading is that it hinders reasoning about the code, which makes me disagree about
It has a more natural FP feel
actually it goes against currying, to me it has a more C#ish feel.
Then, for the same reasons, there is the potential problem that the compiler takes an undesired overload when type information is not present.
Reacted by Abel Braaksmais that it hinders reasoning about the code,
Yes, this is certainly a downside. But the same could be said for the overload with the tupled arguments, as used in
Async. I only found out after years of using it that it even existed.actually it goes against currying, to me it has a more C#ish feel.
How? It is a function that takes an argument that returns a new one-argument function, like
'a -> 'b -> 'c), essentially. Or, in implementations, likelet f x = fun y -> z. How is that non-functional?there is the potential problem that the compiler takes an undesired overload when type information is not present.
Again, how? These overloads are deterministic. Wrong overload resolution only happens if the types overlap. Here that's never the case, as the overloaded function is the one with the
CancellationToken.I'm not saying that there aren't any downsides, but the
Asyncapproach, where there is a mysteriously defaulted second half of a tuple, is rather surprising as well, and what is more, it leads to much more verbose code:type TaskSeq = static member isEmpty(source: Task<bool>, ?token: CancellationToken) -> Task<bool> = Internal.isEmpty(source, token) // example of using it module Test = // this is what I don't like, esp in pipe-chains, or scenarios where simple currying would otherwise suffice // i.e., you cannot use the cancellation token with currying at all, you have to create a new function let f() = TaskSeq.empty |> fun ts -> TaskSeq.isEmpty(ts, CancellationToken.None) let g() = TaskSeq.empty |> TaskSeq.isEmpty
Anyway, I definitely agree it is a novel approach and as such it may raise some eyebrows... 🤨
Currying states that given a function
't1 -> 't2 -> 't3when't2is not supplied the result becomes't2 -> 't3.
Here this reasoning is broken as it becomest3.When the F# compiler is solving types, it tried to do overload resolution, even if type information is not fully nominalized it will attempt to choose a solution, so if you're using this in a generic function it's likely that it will pick an undesired overload or force you to type annotate the function.
The Async approach uses optional arguments to a method, which doesn't interfere with currying.
🤖 This is an automated response from Repo Assist.
Chiming in with a status check, since this issue is still open in milestone v0.5.0 despite the 0.6.x train moving forward.
Where the discussion landed (2023)
The thread reached a clear consensus:
- Preferred approach: optional
CancellationTokenoverloads (theAsyncmodule pattern), rather than the CEdo!/setCancellationTokenidea. @dsyme's quote: "In short, I believe this is the only correct/viable solution."- The curried
TaskSeq.isEmpty (ct: CancellationToken) : taskSeq<'T> -> Task(bool)alternative was explored but rejected (type-overload confusion, breaks currying reasoning).
What still needs doing
The agreed shape for a function like
isEmptywould be:type TaskSeq = static member isEmpty: source: TaskSeq<'T> -> Task(bool) static member isEmpty: token: CancellationToken -> source: TaskSeq<'T> -> Task(bool)
…mirroring the pattern used by
Asyncmodule overloads. This would need to be done for every function that returns aTask<_>(i.e. the "consuming" functions:fold,iter,reduce,length,head,contains,exists,find,toArray, etc.).This is a non-trivial but mechanical change: for each function, add an overload that passes the token through to
GetAsyncEnumerator. TheIAsyncEnumerable<'T>implementations produced bytaskSeq { }already honour the token passed toGetAsyncEnumerator(fixed in #311).Possible next steps
- Narrow scope for 0.6.x: ship the overloads only for the most-requested functions first (e.g.
fold,iter,toArray/toList). - Separate PR per function group to make review easier.
- Update milestone: if this won't land in 0.5.0/0.6.x, moving to
vFutureavoids misleading the milestone tracker.
Happy to attempt a PR for any scoped subset of functions if that would be helpful.
Generated by Repo Assist · ◷
To install this agentic workflow, run
gh aw add githubnext/agentics/workflows/repo-assist.md@346204513ecfa08b81566450d7d599556807389f- Preferred approach: optional
Currently, it is not very clear how users can pass a
CancellationTokenthrough. While the CE has support for cancellation tokens, and the token is passed on toGetAsyncEnumerator(ct), unless users access the interface directly, there is currently no way to pass a cancellation token.There are four, not necessarily exclusive ways, to implement this.
TaskSeq.setCancellationToken, which will return ataskSeqwith the given cancellation tokendo!overload that allows writingtaskSeq { do! myCancelToken }in your CE, otherwise behaving the same as (1)taskSeq { cancellationToken myCancelToken }in your CEtaskSeqCE constructor. However, if possible, this may not be easily discoverableThere are upsides and downsides to each of these approaches. I think the first option, together with a
getCancellationTokenshould at least be supported.However, there are other challenges as well. The helper functions in the
TaskSeqmodule all use theCancellationToken()constructor (that is: no cancellation support). ThetaskSeqbuilder in that respect is a bit of a mixed beast. Yes, you can pass in a cancellation token, but you have to do it manually, and then, using any of the helpers will basically ignore this token.That's not a good position to be in. Probably, code like
source.GetAsyncEnumerator(CancellationToken())should become something likesource.GetAsyncEnumerator(TaskSeq.getCancellationToken source). Which runs into another issue: while the state machine has a property for the cancellation token, any other implementation ofIAsyncEnumerable<'T>does not, which requires a type check to detect.Adding all this magic comes at a cost. In
AsyncSeqandAsyncthis was resolved by adding overloads that take an optional cancellation token. Not ideal either.