Feature request: Add HandleResultAsync

[larsbe]

Hey everybody,

today I ran into a case where handling a result involved executing an async operation:

var policy = Policy<HttpResponseMessage>
                .HandleResult(response => response.Content.ReadAsStringAsync().Result.Contains("pattern"))
                .RetryAsync(retries, (exception, retryCount, context) => doSomething());

So as you can see, I need to access the content of the response to decide whether to retry or not. Unfortunately the only way to parse it is with async methods. So to get it to work with HandleResult, I need to add Result, which is not recommended and might lead to problems.

The obvious solution would be to add a HandleResultAsync method to Polly, similar to e.g. Retry and RetryAsync. I haven't looked at the code myself yet, but maybe it's not too complicated to add the async counterpart and I think it would enable support for a quite frequent use case.

Anyways, I love using Polly. It makes things a lot easier, so thanks for that! :)

[reisenberger]

Hi @larsbe . This is a great question! Makes total sense in terms of Task<HttpResponseMessage> HttpClient.GetAsync(...) and Task<string> HttpResponseMessage.Content.ReadAsStringAsync() being a two-stage async process.

So: it would be possible to add HandleResultAsync(...) (adding Task<bool> versions of the ResultPredicates to Polly). The main question (last time we considered) was that it encourages moving substantive "work" (eg I/O work) into the handle clauses. The question then arises: what should the policy do if the .HandleResultAsync(...) clause (now doing substantive I/O) itself throws an error? For example, with:

.HandleResultAsync(async response => (await response.Content.ReadAsStringAsync()).Contains("match"))

what should the policy do if a failure (disconnection/timeout) occurs during the ReadAsStringAsync()? Does it just throw it? (/place that error on the returned Task of the ExecuteAsync()?). Can the caller tell then whether a given error placed on the returned Task of ExecuteAsync() occurred during the main .Execute'd delegate, or during one of the .HandleResultAsync(...) clauses? And, how would one express error handling for the .HandleResultAsync()? It could lead to nesting policies within the .HandleResultAsync(), like:

var asyncGetPolicy = Policy<HttpResponseMessage>
    .HandleResultAsync(async response => (await asyncReadPolicy.ExecuteAsync(() => response.Content.ReadAsStringAsync())).Contains("pattern"))
    .RetryAsync(retries, (exception, retryCount, context) => doSomething());

(But it's hard to reason still about where/how the eventual error gets expressed, if asyncReadPolicy doesn't deliver a success result.)

TL;DR: We liked the idea of .HandleResultAsync(...) as an appealing analogue to .HandleResult() too, but we didn't find a clear/simple enough path to express its error-handling (suggestions from anyone?).

---

As such, when faced with this kind of case, I've tended to take the two-stage calls of the original:

HttpResponseMessage responseMessage = await httpClient.GetAsync(url);
string content = await responseMessage.Content.ReadAsStringAsync();

and just express a separate policy for each stage:

// (DI, cancellation tokens etc, omitted, to focus on structure) 

RetryPolicy<HttpResponseMessage> asyncGetPolicy = Policy<HttpResponseMessage>
    .Handle<WhateverExeceptions>()
    .HandleResult(r => r.StatusCode == /* etc */)
    .RetryAsync(/* etc */);
RetryPolicy<string> readStreamPolicy = Policy<String>
    .Handle<WhateverExeceptions>()
    .HandleResult(s => s.Contains("pattern"))
    .RetryAsync(/* etc */);

string response = await readStreamPolicy.ExecuteAsync(async () => 
{
    HttpResponseMessage responseMessage = await asyncGetPolicy.ExecuteAsync(async () => await httpClient.GetAsync(url));
    return await responseMessage.Content.ReadAsStringAsync(); 
}); 

It's longer, but it's clear what each policy will handle/retry. To make it concise to client code, of course you can wrap it into a helper method.

But: Does anyone have a better/alternative pattern to share? As you say, @larsbe, this is a common problem.

[reisenberger]

A more minor issue is we don't at mo guarantee to call the .HandleResult(...) predicates only once. Policy engines use them to check if a result should be handled (example), and ExecuteAndCapture/Async(...) overloads also use them in the 'capture' phase to determine after-the-execution if the result was a handled-result (example). That multiple execution likely wouldn't play with any async I/O (including stream-reading) which people might well put in a .HandleResultAsync(...) clause.

We could easily refactor to remove that multiple execution, but at the minor expense of making every execution an execute-and-capture, and then discarding the 'capture' part again for the execute-only codepaths.

[reisenberger]

Noting an additional angle for future ref: A .HandleResultAsync<TResult>(Func<TResult, Task<bool>>) clause would only make sense with async policies limited to async executions. However, we are also looking at unifying sync and async policies.

If #281 goes through, .HandleResultAsync<>() clauses won't make sense on a policy which (given it could be used for both) can also be used for sync executions. So we would need to keep all async in the .ExecuteAsync() clause, and avoid .HandleResultAsync<>(), if #281 is to go through.

For this reason, I'd recommend instead the approach in my original comment: keep all the async work in ExecuteAsync() clauses.

[larsbe]

Thank you, @reisenberger, for the detailed answer!

You argue for having a two-stage policy, however, this feels a bit like a workaround for me (at least in my case). Here is why:

Your main argument is about the error handling and especially the unclear origin of exceptions, if you have a HandleResultAsync<>() method. But if you consider your proposed solution of a two-staged policy, a potential exception in the second stage would then also originate from the ExecuteAsync<>() method. So, I agree that error handling is problematic, but I don't see advantages in your proposed solution.

I do agree that a two-stage policy is the right way to go, if you have a complex operation inside a potential HandleResultAsync<>() method. But this is not the case for me. I actually don't want a separate policy for ReadAsStringAsync(). If this operation fails, just let it fail and return me an exception right away.

Regardless, thanks for the example you gave me. It definitely helps my case and I am going to implement it this way for now, but it just feels unnecessary complex for my needs. So, you might still want to consider my feedback!

[reisenberger]

Closing as 'wont implement' for the reasons discussed earlier in this thread.

At the moment in Polly, there is a clear distinction between lightweight .Handle<>() clauses (which don't need guarding for error), and the .Execute() clauses (which do the heavy work, and which the policies guard for error).

Adding a .HandleResultAsync<>() would start to move heavy work, needing guarding for error, into handle clauses. This blurs the clarity of how Polly operates, and leads to complex reasoning about what happens when an exception is thrown within the handle clause.

[reisenberger]

For anyone exploring this problem, this Stackoverflow answer explores a possible solution to a similar case.

[rmandvikar]

With nested retry policies, wouldn't it retry retryCount ^ 2 times instead of just retryCount times?