-
Notifications
You must be signed in to change notification settings - Fork 8
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
CancellationToken not used/registered #179
Comments
I could've sworn I answered this a while ago, while I was still out sailing. Anyway, I am back now and I will pick this up again after some housekeeping and fixing some low hanging fruit. |
I had the idea to make a registration on the cancellation token, and setting an exception on promiseOfValueOrEnd there. There would be some book-keeping with disposing the registration, but nothing too big. |
Note that your example is currently not using a cancellation check. While we are working on better support for cancellation tokens (see, for instance, #78, #133 and #184 for some background), these won't solve your issue, as inside your implementation you will need to decide how often and when you will check for the cancellation token. let infinite () = taskSeq {
while true do
yield 1 // this is essentially a synchronous loop, and there's no check on cancellation here
} Now, we could certainly consider checking for the cancellation token inside the CE, for instance during For instance, consider a scenario where you need to pull pairs of data, and you coded that such that after pulling each two items, you check for cancellation. If then this library would throw in the middle of these two All that said, we do need to make the cancellation token accessible from inside the CE, which currently isn't the case. |
I understand that it is not a given which semantics are the most desirable. Especially since normal tasks and task-likes do not provide any natural hook for cancellation. While I'm not the biggest fan of the C# approach with ConfiguredCancelableAsyncEnumerable (which would require some work to support, as it does not implement IAsyncEnumerable) maybe something similar is a reasonable solution. E.g. a function or extension that returns a special task-like, that can accept a cancellation token when bound. Or maybe merge with IcedTasks, as I have seen suggested elsewhere. |
This is in the pipeline, see #167.
Merge, no, these libraries support very different use cases. Support, yes. It was indeed discussed, see #188.
All functions that consume the input sequence will get an overload to accept a cancellation token. We will also update the TLDR: we will very much support cancellation in full, to the same extend that My point in the previous comment, however, is that none of this will change your use case: your code is impossible to cancel, because you didn't write cancellation support in your own code. Now, that's understandable, given that you currently cannot access the cancellation token from within the |
Hi.
Thanks for working on this. I have been exploring this repository since I wanted to do something similar, and I noticed that even when manually passing in a cancellation token, it is not directly acted upon. So for example
will run forever. However, if something like
Async.Sleep()
is bound inside the taskSeq, it will end with an exception, since the token is actually passed to the async when it is bound.Do you think it would make sense to set the
promiseOfValueOrEnd
to a cancelled exception (which seems to be the ideomatic dotnet way) or complete(false) when the token is cancelled? I.e, using the CancellationTokenRegistration to abort MoveNextAsync on cancellation?The text was updated successfully, but these errors were encountered: