Maybe add extension methods which still take Tasks instead of ValueTasks?

[Tornhoof]

In V8 the ResiliencePipeline.ExecuteAsync methods now take Func<CancellationToken, ValueTask>().
I personally expect (atleast in my code) that any call which actually requires some kind of retry, let's say because of timeout, actually finishes asynchronously and with that is a Task and not a ValueTask.
I don't want to argue the decision to use ValueTask, I'm just suggesting to add extension methods/overloads or similar with direct support for Task, it just makes the code look a bit cleaner if using Task.

With ValueTask;

await myPipeline.ExecuteAsync(async c => await MyMethod(c).ConfigureAwait(false), cancellationToken).ConfigureAwait(false);

vs.
with Task:

await myPipeline.ExecuteAsync(c => MyMethod(c), cancellationToken).ConfigureAwait(false);

Having an extension for Task should also skip one of the asyncmethodbuilder constructs.

In my own codebase i just added the following two extension methods:

public static ValueTask ExecuteAsync(this ResiliencePipeline pipeline, Func<CancellationToken, Task> callback, CancellationToken cancellationToken = default) =>
            pipeline.ExecuteAsync(c => new ValueTask(callback(c)), cancellationToken);

public static ValueTask<TResult> ExecuteAsync<TResult>(this ResiliencePipeline pipeline, Func<CancellationToken, Task<TResult>> callback, CancellationToken cancellationToken = default) =>
            pipeline.ExecuteAsync(c => new ValueTask<TResult>(callback(c)), cancellationToken);

[martintmk]

Thanks for the feedback. Just for the context, at some point we supported both value task and task. After the API review, the value task won.

Here is the PR:
#1194

Currently, we will be only supporting value task. If there is an overwhelming desire to add task-based overloads we can do it later.

In the end it's only minor inconvenience to make the callback async and call task-based APIs.

[akade]

What was the rationale in behind the decision?

We are currently updating to v8, while it's no issue to migrate to ValueTask for us, I'm not sure anymore if my mental model is correct. I used as a rule of thumb the following heuristics to decide between Task & ValueTask:

Only use a ValueTask if the method usually runs synchronously but may occasionally run asynchronously. For Polly, at least the way we use it, that is almost never the case. The network call is always allocating a Task anyway. That's why I'm curious.

(Btw, thanks for the amazing work with Polly!)

[martintmk]

I am not expert in this area, but using ValueTask gives some leverage towards optimalizations in the future. Task will always allocate, while for ValueTask you can use more advanced patterns to get rid of allocation even in async case.

For example:
https://devblogs.microsoft.com/dotnet/async-valuetask-pooling-in-net-5/

[akade]

Thanks for the reply. You are right I think, I forgot about the potential for pooling the underlying Task due to the ValueTask not supporting "reuse" anyway... Thanks a lot for the answer :).