CancellationToken not used/registered

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

let consumeManually (enumerable: IAsyncEnumerable<int>) (token: CancellationToken) = task {
    do! Task.Yield()
    let mutable hasRead = false
    use enumerator = enumerable.GetAsyncEnumerator(token)        
    let! canRead = enumerator.MoveNextAsync()
    hasRead <- canRead
    while hasRead do
        let _here = enumerator.Current
        let! canRead = enumerator.MoveNextAsync()
        hasRead <- canRead

    return ()
}

let infinite () = taskSeq {        
    while true do
        yield 1
}

[<Fact>]
let usesCancellationToken () = task {
    use src = new CancellationTokenSource()
    let readToEnd = consumeManually (infinite()) src.Token
    do! Task.Delay(100)
    src.Cancel()
    do! readToEnd 
}

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?

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 MoveNext, but tbh, I am not certain it is wise to abort that way.

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 MoveNext operations because someone canceled, it would break your application. Perhaps we could support this scenario optionally, though.

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.

approach with ConfiguredCancelableAsyncEnumerable

This is in the pipeline, see #167.

Or maybe merge with IcedTasks, as I have seen suggested elsewhere.

Merge, no, these libraries support very different use cases. Support, yes. It was indeed discussed, see #188.

that can accept a cancellation token when bound

All functions that consume the input sequence will get an overload to accept a cancellation token. We will also update the Async<'T> support such that we will pick its cancellation token, plus add more overloads to support Async (not to be confused with Task) out of the box.

TLDR: we will very much support cancellation in full, to the same extend that task does (since it is its natural equivalent) and where applicable, a little bit better.

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 taskSeq computation expression, and that's yet another change that is very much in the pipeline.

fix: honor CancellationToken in MoveNextAsync (closes #179)

Merge pull request #311 from fsprojects/repo-assist/fix-cancellation-…

Navigation Menu

Repository navigation

CancellationToken not used/registered #179

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

CancellationToken not used/registered #179

Description

Activity

abelbraaksma commented on Oct 29, 2023

JohSand commented on Oct 29, 2023

abelbraaksma commented on Oct 31, 2023

JohSand commented on Nov 1, 2023

abelbraaksma commented on Nov 2, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions