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

Support ConfiguredCancelableAsyncEnumerable<'T> #167

Open
xperiandri opened this issue Jun 21, 2023 · 5 comments
Open

Support ConfiguredCancelableAsyncEnumerable<'T> #167

xperiandri opened this issue Jun 21, 2023 · 5 comments
Labels
feature request New feature or enhancement request topic: surface area Adds functions to the public surface area
Milestone

Comments

@xperiandri
Copy link

When you call .WithCancellation(cancellationToken) on IAsyncEnumerable<'T> you get System.Runtime.CompilerServices. ConfiguredCancelableAsyncEnumerable<'T> which is not currently supported.

So that this code does not work

    static member ToFlatListAsync<'Source>(source: IQueryable<'Source>, [<Optional>] cancellationToken: CancellationToken) = task {
        let builder = ImmutableArray.CreateBuilder<'Source>()
        do!
            source.AsAsyncEnumerable().WithCancellation(cancellationToken)
            |> TaskSeq.iterAsync (fun x -> builder.Add(x))

        return builder.ToImmutable();
    }
@abelbraaksma
Copy link
Member

abelbraaksma commented Aug 3, 2023

This is actually very interesting. Reading up on the design decisions for WithCancellation, as Don Syme and myself were about creating our own solution, but it is, obviously, better to use dotnet's own methods, if they are applicable.

Just checked, this method is part of NetStandard 2.1, so I can actually add it. Another method in those extensions that is very interesting is ToBlockingEnumerable. This is a .NET 7+ extension, but I might take its code for the blocking operations I have, to ensure similar behavior.

@abelbraaksma
Copy link
Member

Related discussion: #133

@abelbraaksma abelbraaksma added topic: surface area Adds functions to the public surface area feature request New feature or enhancement request labels Oct 29, 2023
@abelbraaksma
Copy link
Member

abelbraaksma commented Oct 29, 2023

This turns out to be much harder to support than I thought. But one of the soon-to-be-made changes is going to be a move to static methods, so we can support adding a cancellation token in each call immediately, removing the need for calling WithCancellation, but that means threading an overload through the whole system.

To support this, we either need to overload every single method such that it accepts that type, or we have to go SRTP (and/or with an SRTP overload).

Instead, I may consider something like ofWithCancellation (ugly method name, I know), which would itself return an IAsyncEnumerable but with a collected cancellation token. This is, however, not ideal, as the token should be given to GetEnumerator(token). Hmm, I'll think a bit more about this.

The reason this is non-trivial is because ConfiguredCancelableAsyncEnumerable<T> is a struct and does not implement any useful interface. I'm also a little confused as to why MS chose to internally effectively call ConfigureAwait(true), which, from library code, is considered bad practice in general (and can lead to deadlocks).

@xperiandri
Copy link
Author

Maybe raise an issues on .NET repo about the use of ConfiguredCancelableAsyncEnumerable<T>?

@abelbraaksma
Copy link
Member

abelbraaksma commented Nov 4, 2023

That likely wouldn't help. It is a struct for a reason. While it keeps a reference to the IAsyncEnumerable<_> internally, it makes the overhead of making your enumerable cancelable really small. There's no need for a newobj IL instruction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or enhancement request topic: surface area Adds functions to the public surface area
Projects
None yet
Development

No branches or pull requests

2 participants