Polly icon indicating copy to clipboard operation
Polly copied to clipboard

WaitAndRetryAsync does not contain possibility to continueOnCapturedContext

Open simluk opened this issue 4 years ago • 4 comments

We are trying to avoid having configure awaits that switch the context in our application. Apparently Polly defaults to false if you are not specifying it explicitly.

WaitAndRetryAsync however does not have an overload to override continueOnCapturedContext to true. Are there any options to workaround this or maybe you could provide a way of doing it in next release?

Simonas

simluk avatar Jan 22 '21 10:01 simluk

.NET guidance is that libraries always specify ConfigureAwait(false), so as far as I'm aware that's what we do as Polly is a library. There's a great blog post by Stephen Toub on the .NET team here.

The specific call-out for this is quoted below from this section:

if you’re writing general-purpose library code, use ConfigureAwait(false)

It's not scalable for every async API everywhere (not just in Polly, but throughout the .NET runtime) to have parameters to do this. If you need to use true, you should specify that in the appropriate points in your own code when you call async methods.

martincostello avatar Jan 22 '21 11:01 martincostello

It's not scalable for every async API everywhere (not just in Polly, but throughout the .NET runtime) to have parameters to do this. If you need to use true, you should specify that in the appropriate points in your own code when you call async methods.

I would agree with you, but since you already have extension points in ExecuteAndCaptureAsync and ExecuteAsync overloads with continueOnCapturedContext flag I don't see why it would be hard to extend this further? This seems very inconsistent API, and consistency I think is what makes a good library. As of your comment, we are specifying true where we need, but we cannot use Polly everywhere we want because of this issue. If we do we are basically losing the MVC HttpContext and sometimes we get errors that depend on the context with subsequent code. We already registered one bug and solved it in regards to that: https://github.com/App-vNext/Polly/issues/674

Let me know if you need further details or explanation on the issue. Thanks for quick reply.

Simonas

simluk avatar Jan 22 '21 12:01 simluk

We can see what other people think in the team and community, I'm just giving my own opinion, but adding lots of extra options in all the different overloads for all the different policies that have async will lead to even more combinations of code paths and configuration to deal with that isn't strictly necessary, increasing the amount of things to maintain and test.

The number of parameters on policies is already causing pain and is being considered to be revisited for Polly v8, so maybe something like this would be more workable in a rethink of how configuration works, but at the moment I think it would probably cause a lot of extra maintenance burden and complexity relative to the gain.

In the issue you reference, that's a bug in functionality that existed already. This seems to be about extending it further, which is where the maintainability argument comes in on whether that's the right thing to do.

martincostello avatar Jan 22 '21 12:01 martincostello

The number of parameters on policies is already causing pain and is being considered to be revisited for Polly v8, so maybe something like this would be more workable in a rethink of how configuration works, but at the moment I think it would probably cause a lot of extra maintenance burden and complexity relative to the gain.

I agree that having that amount of parameters to Execute methods seems like an overkill. Maybe it's worth allowing to build configuration in following manner: return Policy .ContinueOnCapturedContext(true) .HandleResult<HttpResponseMessage>(response => !response.IsSuccessStatusCode) .Or<Exception>() .WaitAndRetryAsync(_maxRetryAttempts, SleepDurationProvider);

That would allow avoiding arguments to Execute, but not sure how much refactoring it would take to actually do it.

I am hearing your opinion though. We'll switch from Polly for now with own retry mechanism to workaround it for now.

Simonas

simluk avatar Jan 22 '21 13:01 simluk

This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made.

github-actions[bot] avatar Jul 04 '23 02:07 github-actions[bot]

This issue was closed because it has been inactive for 14 days since being marked as stale.

github-actions[bot] avatar Jul 18 '23 02:07 github-actions[bot]