Additional ExecutionStrategy extension point: OnOperationRetrying(Async)

Open
#12,536 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
35/100
Issue type
Feature
Clarity
Mostly clear
Activity status
Stale
Tech stack
csharp
Domain
backend, databases

Research direction

Start in src/EFCore/Storage/ExecutionStrategy.cs, reading OnRetry and the ExecuteImplementation and ExecuteImplementationAsync entry points. Trace the existing retry and delay flow, including how ShouldRetryOn handles exceptions. Done means the proposed synchronous and asynchronous retry extension points fit both flows without disrupting existing behavior, with coverage for their timing and exception handling.

Written by the indexing model from the issue text.

Description

area-save-changes customer-reported

In ExecutionStrategy, there is a convenient template method OnRetry that will be called after an operation failure. However, there are a few limitations to this extension point:

  1. It is called before the delay, so you cannot do something after the delay (i.e. allowing the database time to recover)
  2. Any exceptions thrown by OnRetry are not caught and handled via ShouldRetryOn like operation exceptions are
  3. It does not have an async version

I'd like to propose an additional extension point, which for the sake of this issue I'll call OnOperationRetrying (feel free to rename). My primary use case is to be able to execute some SQL queries before the operation in the case of a retry, but I could imagine other use cases such as logging or consistency validation.

It would be a similar template-method signature to OnRetry but with an additional async version:

protected virtual void OnOperationRetrying() 
{
}

protected virtual Task OnOperationRetryingAsync(CancellationToken cancellationToken)
{
    return Task.CompletedTask;
}

This would be called from ExecuteImplementation and ExecuteImplementationAsync inside the try block, immediately before the call to operation. For example:

        private TResult ExecuteImplementation<TState, TResult>(
            Func<DbContext, TState, TResult> operation,
            Func<DbContext, TState, ExecutionResult<TResult>> verifySucceeded,
            TState state)
        {
            bool isRetrying = false; // This is new
            while (true)
            {
                TimeSpan? delay;
                try
                {
                    Suspended = true;
                    if (isRetrying) {
                        OnOperationRetrying(); // new extension point
                    }
                    var result = operation(Dependencies.CurrentDbContext.Context, state);
                    Suspended = false;
                    return result;
                }
                catch (Exception ex)
                {
                    ... // existing catch code unmodified

                    OnRetry(); // this will still exist
                    isRetrying = true; // this is new
                }

                using (var waitEvent = new ManualResetEventSlim(false))
                {
                    waitEvent.WaitHandle.WaitOne(delay.Value);
                }
            }
        }

And of course there would be similar logic in ExecuteImplementationAsync just using await OnOperationRetryingAsync() instead.

This addresses all 3 of the limitations above:

  1. It is called right before the next operation, so it is after the delay
  2. It is inside the try block, so any exceptions that occur that ShouldRetryOn says should be retried will be retried
  3. There is an async version

In addition, this should not impose a significant performance penalty in the common (non-retry) case, as only an if check of a boolean variable is added on the first execution of the loop. And because of the delay logic, the additional cost of a non-overridden virtual method call on a retry is negligible.

The only caveat that would be needed for the documentation would be that if OnOperationRetrying throws, that would count as a retry and operation would not be called. But by its nature, that is the expected behavior. (If OnOperationRetrying fails, the expectation is that operation would likely fail anyways.)

I'd be happy to submit a PR for this, but I wanted to get feedback on it first to see if it's acceptable, or if someone has an alternative of a better way of doing this (without having to copy/paste reimplement ExecutionStrategy).

Dominant language
C#
Stars
14.8k
Forks
3.4k
Avg merge
2d 5h
Merged PRs (30d)
134

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from dotnet/efcore

All issues in dotnet/efcore

Similar issues

More C# issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.