Delay first retry in Transient Error Handling with Azure SQL

[martyt]

The published advice on transient error handling for Azure SQL recommends

• Delaying "several" seconds before the first retry
• Closing and opening the connection prior to eeach retry

I've poked around in SqlServerRetryingExecutionStrategy and related classes, and can't find any evidence that either of those two recommendations are followed in EF Core - nor have I been able to figure out how I might implement those recommendations in a custom execution strategy.

Additionally, I've found that an Execution Timeout Expired. exception (Error number -2) is explicitly not considered transient -- yet it is the single most frequently occurring exception we encounter in our non-EF database code. The retry strategy we've implemented for that non-EF code closes and re-opens the connection before retrying the query and has completely eliminated failures due to timeout exceptions. I've had to add error number -2 to the errorNumbersToAdd list for EF Core, but, because the connection isn't closed and re-opened, I have zero expectation that retries for those errors will be successful.

Is there a plan to support the recommended transient error handling when targeting Azure SQL? Is there a way I can implement a custom execution strategy that will close and re-open the database connection?

[AndriySvyryd]

You can change the delay between retries by overriding GetNextDelay

In most cases a retriable exception puts the connection in a Broken state, so it will be closed and reopened

The main reason for not retrying timeouts is that they could be caused by a query that uses too many resources, and retrying by default would hide this issue.

[martyt]

@AndriySvyryd Thanks for the reply. Since my original post, I've implemented a custom ExecutionStrategy that explicitly closes the connection, waits 5 seconds and then re-opens it (via an override of OnRetry()) - seems to be working fine but if I don't need to go to all that trouble, I'm definitely interested in changing my implementation.

In most cases a retriable exception puts the connection in a Broken state, so it will be closed and reopened

Can you point me to the code that sets the connection state to Broken when a retriable exception occurs? I dug through the EFCore repo and couldn't find anything obvious. I'd like to confirm exactly what happens on an exception and modify my code accordingly.

[AndriySvyryd]

Can you point me to the code that sets the connection state to Broken when a retriable exception occurs?

That's handled by SqlClient

[ajcvickers]

Note for triage: we should check the latest recommendations as part of the 7.0 process, since these have changed since the feature was implemented.

[roji]

Keep in mind that different error types may need different timeouts. For example, while availability-related errors may justify a longer timeout (to allow the service to become available again), a deadlock or concurrency-related error should probably be retried immediately (possibly even adding a bit of random jitter to increase the chances it isn't reproduced).

[ajcvickers]

Note from triage: also consider information here: https://github.com/dotnet/efcore/issues/28200

[MNF]

. Since my original post, I've implemented a custom ExecutionStrategy that explicitly closes the connection, waits 5 seconds and then re-opens it (via an override of OnRetry()) - seems to be working fine but if I don't need to go to all that trouble, I'm definitely interested in changing my implementation.

@martyt could you please share your custom ExecutionStrategy implementation?

[martyt]

@MNF this is what we're using now - I removed all the extra logic to close and reopen the connection since it turned out to be unnecessary. The only things that this strategy does that's different from the built-in is to use the DecorrelatedJitterBackoffV2 method (in the Polly package) and treat SQL Timeout (-2) errors and IOException and SocketException as transients that should be retried.

/// <summary>
/// A custom sql execution strategy for EntityFramework that utilizes the DecorrelatedJitterBackoffV2 method from Polly
/// to calculate the delay before next retry.
/// </summary>
public class CustomEfCoreSqlExecutionStrategy : SqlServerRetryingExecutionStrategy
{
    private readonly IEnumerable<TimeSpan> _backoffDelays;

    public CustomEfCoreSqlExecutionStrategy(
        ExecutionStrategyDependencies dependencies,
        int maxRetryCount,
        TimeSpan maxRetryDelay,
        ICollection<int> errorNumbersToAdd
    ) : base(dependencies, maxRetryCount, maxRetryDelay, errorNumbersToAdd)
    {
        _backoffDelays = Backoff.DecorrelatedJitterBackoffV2(TimeSpan.FromSeconds(5), maxRetryCount);
    }

    /// <summary>
    /// Get the delay before the next retry using Polly's DecorrelatedJitterV2 implementation
    /// </summary>
    /// <param name="lastException"></param>
    /// <returns></returns>
    protected override TimeSpan? GetNextDelay(Exception lastException)
    {
        var currentRetryCount = ExceptionsEncountered.Count - 1;
        if (currentRetryCount < MaxRetryCount)
            return _backoffDelays.ElementAt(currentRetryCount);

        return null;
    }

    /// <summary>
    /// Experience has shown that we frequently encounter SqlExceptions that have an unknown error number but have an
    /// IOException and/or SocketException as the inner exception. So we need to treat those as transient.
    ///
    /// Also, the default EF Core retry strategy explicitly excludes SQL Timeout errors (-2), but those errors are the most frequent
    /// that we see in the wild. 
    /// </summary>
    /// <param name="exception"></param>
    /// <returns></returns>
    protected override bool ShouldRetryOn(Exception exception)
    {
        // all i/o exceptions are considered transient
        // error code -2 (timeout) is also transient, even though the base implementation says otherwise
        var shouldRetry =
            exception is SqlException {InnerException: IOException or SocketException}
                or SqlException {Number: -2} || base.ShouldRetryOn(exception);

        return shouldRetry;
    }
}

[ajcvickers]

Note for triage: proposing we punt on this for 7.0, since I'm not sure changing delays so close to the release is a great idea. We should instead make this change early in 8.0 so we have a chance to react to any incoming changes in reliability or performance characteristics.

[stevendarby]

A few questions...

@ajcvickers is this still on the radar for 8.0 given the last comment above?

The advice highlighted in this issue seems to be specifically about Azure SQL; is there an existing mechanism EF can use to detect when it's connecting to Azure SQL and not on-premise SQL Server? I'm wondering if this issue is more of an enhancement than a bug.

Since handling of -2 has also come up a few times in this issue, I have some questions/comments on that too.

The code has this comment: https://github.com/dotnet/efcore/blob/16c1380d3bd839b6adee586bc990797fbdb64597/src/EFCore.SqlServer/Storage/Internal/SqlServerTransientExceptionDetector.cs#L199-L202

@AndriySvyryd also said above that the reason not to retry is because the query might be using too many resources. This article says timeout exceptions can be due to connection or query issues and provides a way to differentiate. Perhaps EF needs a mechanism for determining if -2 should be retried. We get a fair few of these with Azure SQL and on investigation they seem to be connection related so could probably have been retried, but we don't want to just add -2 to the list in case it's the query kind. Then there's also the comment in the code about it possibly being successful which adds further doubt around the matter.

[AndriySvyryd]

The advice highlighted in this issue seems to be specifically about Azure SQL; is there an existing mechanism EF can use to detect when it's connecting to Azure SQL and not on-premise SQL Server?

Not currently. It is recommended to use different Execution strategies depending on the target database.

This article says timeout exceptions can be due to connection or query issues and provides a way to differentiate. Perhaps EF needs a mechanism for determining if -2 should be retried.

Adding this would be out of scope for this issue. I've opened https://github.com/dotnet/efcore/issues/30023 to track it.

[stevendarby]

@AndriySvyryd could you help me understand what a bug fix for this current issue might look like?

[AndriySvyryd]

@stevendarby We'll probably add an Azure-specific execution strategy that the user needs to choose explicitly

[stevendarby]

@AndriySvyryd can I just check my reading of the changes around -2 is correct: it hasn't been added to the default list of codes to retry, however if the user has added that as an additional code, it will understand that as throttling error and applies different delays? All good if so. My first read had me slightly concerned that -2 was added so just wanted to check.

[AndriySvyryd]

@AndriySvyryd can I just check my reading of the changes around -2 is correct: it hasn't been added to the default list of codes to retry, however if the user has added that as an additional code, it will understand that as throttling error and applies different delays?

That's correct, -2 is still not retried by default.