Is RetriesOnFailure on the IExecutionStrategy interface poorly named or even misleading?

[pjh1974]

I have written a custom implementation of IExecutionStrategy for the purpose of detecting exceptions that should be retried and wrapping the exceptions so that they can later be handled by a Polly policy. I did look at using the SqlServerRetryingExecutionStrategy but I have a number calls that use distributed transactions and as such cannot wrap the work in a .CreateExecutionStrategy().Execute(() => { ... }), so that isn't going to work. Handling the exceptions higher-up in my code allows me more control over how the retry is performed.

My first implementation of a custom IExecutionStrategy worked fine except in certain scenarios, e.g. Deadlocks from from a DataReader weren't being caught in my strategy. Further investigation led me to the fact the returning True from RetriesOnFailure fixed the problem, due to it triggering the use of a BufferedDataReader to technically read ahead and raise the deadlock exception within the strategy's ExecuteAsync call.

I now have a working implementation of my strategy but it feels like my code is lying by returning True from RetriesOnFailure even though it never actually retries within the strategy. As far as I can tell, the only behaviour that this currently drives is the use of the BufferedDataReader, which is the behaviour that I want, even though I will never retry.

Could RetriesOnFailure have been named to better indicate what it actually does? It seems that there are more uses for a custom IExecutionStrategy that doesn't retry but would still want the BufferedDataReader behaviour.

EF Core version: 6.03
Database provider: N/A
Target framework: NET 6.0
Operating system: N/A
IDE: N/A

[roji]

The intended meaning of RetriesOnFailure is whether the execution strategy implementation retries or not. Other parts of EF Core may react to this information. Specifically, when executing a query under a retrying execution, the query pipeline first buffers the entire query results, since if an error occurs at some point, the query would need to be executed again; and if that happens when user code has already consumed some results from the 1st execution, inconsistency may occur. To avoid this, the entire results are buffered, and user code is only given the first result row after being sure that all results have been loaded and no exception will occur.

In other words, if you're using the RetriesOnFailure to trigger the buffering behavior, you're very likely abusing the abstraction. Now, stepping back...

I have written a custom implementation of IExecutionStrategy for the purpose of detecting exceptions that should be retried and wrapping the exceptions so that they can later be handled by a Polly policy.

You should generally not be implementing an execution strategy as a way to wrap exceptions. Polly itself allows you to provide a policy that determines which exceptions should be retried and which shouldn't - that should be the preferred way to do that (see the Polly docs). Another option would be to implement an EF Core interceptor, but that's a less recommended alternative.

I did look at using the SqlServerRetryingExecutionStrategy but I have a number calls that use distributed transactions and as such cannot wrap the work in a .CreateExecutionStrategy().Execute(() => { ... }), so that isn't going to work. Handling the exceptions higher-up in my code allows me more control over how the retry is performed.

This isn't quite clear - what exactly is preventing you from wrapping your work in CreateExecutionStrategy().Execute()? Execute accepts a lambda, inside which you can do anything at all that you want (with distributed transactions or otherwise); an exception bubbling out would cause the block to be retried.

In fact, Polly works in much the same way, providing an API which accepts a lambda and retrying on (certain) exception (see the Polly Execute code samples). EF Core's retrying execution strategy - when using CreateExecutionStrategy().Execute() - is conceptually equivalent to Polly and works at the same level. I'm definitely not trying to get you to use SqlServerRetryingExecutionStrategy rather than Polly: Polly is awesome and has many more features for what to do when a failure occurs. But you seem to think that Polly is somehow more compatible with your distributed transactions mechanism than SqlServerRetryingExecutionStrategy, which may not be true. A code sample of why you think you can't use SqlServerRetryingExecutionStrategy may help clarify.

[pjh1974]

@roji Firstly, thankyou for your prompt and detailed response.

"Abusing the abstraction" is probably the main reason why I raised this question. I have looked for a better alternative, and to that end I'll address your other points below:

• With regard to the use of CreateExecutionStrategy().Execute(). If there is an ambient transaction, then isn't using CreateExecutionStrategy().Execute() only going to retry the lambda? If the behaviour that I want is to rollback the whole transaction and start again, this cannot be achieved without exposing the DbContext to higher-level concerns, as the CreateExecutionStrategy applies to the DbContext.
• I cannot use an EfCore interceptor because they don't seem to intercept the MoveNext on the DataReader; which is where the Deadlock exception is thrown. This was actually our original solution but we never see deadlock exceptions from the MoveNext of the underlying DataReader.

Apologies, it will take some time to provide a code sample as much of the code is spread over multiple library projects. From your response, it does seem like you think CreateExecutionStrategy is the way to go. I'll do a little more investigation along those lines.

Thanks for your help.

[roji]

With regard to the use of CreateExecutionStrategy().Execute(). If there is an ambient transaction, then isn't using CreateExecutionStrategy().Execute() only going to retry the lambda? If the behaviour that I want is to rollback the whole transaction and start again, this cannot be achieved without exposing the DbContext to higher-level concerns, as the CreateExecutionStrategy applies to the DbContext.

Typically you'd start the ambient transaction (i.e. create the TransactionScope) within the lambda; the idea of the API is execute a self-contained, transactional unit which can be retried as a whole. In any case, there's no difference here between Polly or EF Core's retry strategy - both accept a lambda and retry it if necessary.

You're probably right with regards to the interceptor. But as I wrote above, if the goal is to identify transient exceptions for Polly, do that on the Polly side, not on the EF Core side.

By the way, note that distributed transactions currently aren't supported in .NET 5+ (see dotnet/runtime#715). You can use ambient transactions (i.e. TransactionScope), but only against a single database at a time. I hope you realize that in what you're trying to do.

[roji]

@pjh1974 I'm going to go ahead and close this issue, because I think the questions have been largely addressed. But if you have further questions, don't hesitate to post back here and we'll discuss.