Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for SqlRetryLogic #1864

Open
vakuras opened this issue Dec 12, 2022 · 24 comments
Open

Support for SqlRetryLogic #1864

vakuras opened this issue Dec 12, 2022 · 24 comments

Comments

@vakuras
Copy link

vakuras commented Dec 12, 2022

Hello,

Is there a plan to support the new SqlRetryLogic in dapper?
A way to pass the retry logic to the SqlCommand object?

Thanks

@mgravell
Copy link
Member

My limited understanding (from here is that .RetryLogicProvider can be specified at the connection level; so for that: no changes would be required. At the command level: no, this is not currently supported

@vakuras
Copy link
Author

vakuras commented Dec 12, 2022

Is there a plan to support it?

@mgravell
Copy link
Member

Honestly, today is the first time I've heard of it. Typically, we prefer to target the primary ADO.NET API surface (since we don't want to force any provider-specific dependency trees), and this is very specifically a Microsoft.Data.SqlClient feature, which makes it a little awkward to support. We'd need to think about it, basically.

@billrob
Copy link
Contributor

billrob commented Mar 25, 2023

@vakuras I have had good luck with Polly to easily wrap Sql retry logic. It's syntax is a little awkward to start, but eventually grew on me.

@fsbflavio
Copy link

fsbflavio commented Jul 12, 2023

It would be advantageous to include a retry on fail mechanism in the built-in methods, eliminating the need to create wrappers for all Dapper's methods to handle the errors.

Currently, I am doing something like this:

public new IEnumerable<T> Query<T>(string sql, object param = null)
    {
        var retries = 0;
        while (true)
        {
            try
            {
                // Try to execute the query
                return connection.Query<T>(sql, param); //connection is IDbConnection
            }
            catch (Exception ex)
            {
                // Check if the exception is a network error (replace ORA-12570 with the actual error code)
                if (ex.Number == 'ORA-12570' ) //a list of transient errors
                {
                    if (++retries == _retryCount)
                    {
                        throw; // If all retries failed, propagate the exception.
                    }
                    Thread.Sleep(TimeSpan.FromSeconds(Math.Pow(2, retries))); // Wait before retrying.
                }
                else
                {
                    throw; // If it's not a network error, propagate the exception.
                }
            }
        }
    }
    ```

@billrob
Copy link
Contributor

billrob commented Jul 12, 2023

As you can see from even your own example, there isn't a good way to know when to bubble up and when to retry, then we have a max retryCount to content with.

A DbConcurrencyException will need additional cleanup code before retrying, a SqlException with message of timeout is another behavior.

You should be wrapping all Dapper calls with your own layer nevertheless.

@fsbflavio
Copy link

That's true.
But, it would be useful for Dapper to allow configuring a list of transient error codes/exceptions to retry on, for each database it supports. This could be done in a fluent style, similar to Entity Framework. For example:

public class RetryOnErrorStrategyForOracle : OracleRetryingExecutionStrategy // in Dapper
{
    private static readonly IList<int> ErrorNumbersToRetry = new List<int>()
    {
        12570, 12571, 03135 //lost connection errors
    };

    public SomeRetryStrategy()
        : base(DefaultMaxRetryCount, ErrorNumbersToRetry)
    {
    }
}


//configure Dapper to use the custom strategy in the methods
connection.Query("select * from ...", new RetryOnErrorStrategyForOracle())

Now Dapper could use the configured retry strategy. To keep it simple, it could just use the sample error code list provided here for the OracleRetryingExecutionStrategy implementation.

This would help retry when connection errors occur - I often run into errors 12570, 12571, 03135 when the connection is lost.

OK, Wrapping all the Dapper methods to handle errors could work as a workaround, but modifying Dapper itself to support configurable retry strategies would be a more elegant solution.

Thanks!

@billrob
Copy link
Contributor

billrob commented Jul 13, 2023

I don't want to step on @mgravell and his thoughts for this library. Dapper should do Dapper things, if we want retry features: errors, exceptions, we should extend Dapper with a nuget package of Dapper.Extensions.Retry or something of the sort.

Dapper.dll, shouldn't be littered with DatabaseProviderRetryStrategy code. I like where your head is at for ease of adding retry logic, but not in the Dapper core library.

@mgravell
Copy link
Member

I agree with @billrob's comments above; additionally, a low effort implementation is probably more harmful than helpful. If we you transient retry and resilience - that sounds like a job for Polly, no? Let Dapper do what it knows, and let Polly do what it knows. IMO.

@fsbflavio
Copy link

fsbflavio commented Jul 13, 2023

I understand.
I like the idea of extending Dapper with this option.
An extension library like Dapper.Extensions.Retry would allow implementing retries while keeping Dapper simple.
Polly really works but it is a little awkward to add its logic to all my methods, I end up having my own library that does it.

Thanks for providing that insight

@RyanThomas73
Copy link

Came across this issue while trying to find a way to use SqlRetryLogic. Support for the ability to use the out-of-the-box retries provided by Microsoft.Data.SqlClient would be a big win for me and my team over having to write our own wrapper around dapper and being forced to define the retry policy logic instead of just using the microsoft provided default.

@mgravell
At the core of my request, rather than try to implement provider specific features, can you simply give us an extension point around the command creation (that still adheres to just the abstract ADO.NET API sruface)?

For example, a hypothetical Action<IDbConnection, IDbCommand>? configureCommand = null parameter passed through to the command setup:

var cmd = cnn.CreateCommand();
configureCommand?.Invoke(cnn, cmd);
var init = GetInit(cmd.getType());
... rest of code ...

@billrob
Copy link
Contributor

billrob commented Oct 27, 2024

@RyanThomas73 Doesn't the SqlConnection object allows this already?

https://learn.microsoft.com/en-us/sql/connect/ado-net/configurable-retry-logic-sqlclient-introduction?view=sql-server-ver16

// Assumes that connection is a valid SqlConnection object 

// Set the retry logic provider on the connection instance
connection.RetryLogicProvider = provider;

// Establishing the connection will retry if a transient failure occurs.
connection.Open();

//now make Dapper calls.

@RyanThomas73
Copy link

RyanThomas73 commented Oct 27, 2024

@RyanThomas73 Doesn't the SqlConnection object allows this already?

Yes the connection does, but as mentioned in the above that only helps with connection.Open() call. My request is for an extension point to configure the SqlCommand objects so that we can also use the out-of-the-box retry provider for command execution.

For example, if the CommandDefinition supported an extension point such as an optional action for configuring the DbCommand object as suggested I could simply pass in a delegate, like this:

Action<IDbConnection, IDbCommand> configureCommand = (dbConnection, dbCommand) => {
    ((SqlCommand)dbCommand).RetryLogicProvider = ((SqlConnection)dbConnection).RetryLogicProvider;
}

Such an extension point would
a. Allow us to fully utilize the out-of-the-box features of the Microsoft.Data.SqlClient library
b. Avoid the need for the Dapper library to take on provider specific dependency logic branches
c. Allow other consumers of the Dapper library the ability to use that extension point to leverage other non sql server provider specific functionality that needs to be configured at the DbCommand level

@billrob
Copy link
Contributor

billrob commented Oct 27, 2024

Yes, you are correct. I would have predicted it would have used the retry logic on the connection if it wasn't overwritten on the command. I can see their point for not doing it.

Although I propose to @mgravell Func<IDbConnection, IDbCommand> createCommand the caller would now be in full control of the creation of the IDbCommand. It would be a easyish change to have a default implementation. I can picture other scenarios where more complex IDbCommand creations might come up with the increasing number of db providers out there.

internal static _defaultCreateCommand = new Func<IDbConnection, IDbCommand>((IDbConnection con)
   => con.CreateCommand());

It looks like all code paths end up on

internal IDbCommand SetupCommand(IDbConnection cnn, Action<IDbCommand, object?>? paramReader)

so it would just be some monkey work to add the optional parameter in everywhere.

Would that be more useful?

I suppose the only confusing might be if the caller sets their own command.xxx properties and Dapper's internal implementation changes some of that. That would be frustrating and confusing.

@RyanThomas73
Copy link

RyanThomas73 commented Oct 27, 2024

so it would just be some monkey work to add the optional parameter in everywhere.

The SqlMapper extension methods already have overloads that take the CommandDefinition directly, correct?

Since this is a fairly advanced use case I would argue that it would be sufficient to add such a Func<IDbConnection, IDbCommand>? as a CreateCommand property of the CommandDefinition but leave off adding optional parameters everywhere to minimize additional code the Dapper library needs to maintain.

Consumers that need to leverage this extension point could then create the CommandDefinition directly. e.g.

public async Task DoSomethingAsync(CancellationToken cancellationToken)
{
    using var sqlConnection = ... create and open sql connection ...
    var commandDefinition = new CommandDefinition(commandText, ... other parameters ..., createCommand: CustomCreateCommandDelegate, cancellationToken: cancellationToken);
    var results = await sqlConnection.ExecuteAsync(commandDefinition);
}

@mgravell
Copy link
Member

RyanThomas73 added a commit to RyanThomas73/Dapper that referenced this issue Oct 29, 2024
@RyanThomas73
Copy link

RyanThomas73 commented Oct 29, 2024

I put together a commit with a minimal solution using just a simple function for the command factory, if that helps any:
main...RyanThomas73:Dapper:feature-custom-command-factory

I also took a look at the CommandFactory Marc mentioned from DapperAOT.

The additional extension points beyond just command creation - initialization of the command text and command type, parameter addition, and optional post processing look like they would add lots of customizability but also seem like they would be challenging to retrofit onto the existing codebase/public api for Dapper

@billrob
Copy link
Contributor

billrob commented Oct 29, 2024

Hi @RyanThomas73 I got distracted on working on this because I was getting the docker containers built up to run all the provider tests.

I actually have two approaches I was going to @mgravell with.

This one ctor change is more risky because of the optional parameter with default being passed internally through the Dapper code base.

I also have another branch that has a OverrideCreateCommand(Func<IDbConnection, IDbCommand> createCommand).

I'll defer to Marc here.

Also as note, you put an if check and directly invoke. The performance different is small but still is an order of magnitude slower.

Method Mean Error StdDev Median Allocated
Invoke 0.0225 ns 0.0235 ns 0.0252 ns 0.0153 ns -
InvokeDirect 0.0022 ns 0.0039 ns 0.0036 ns 0.0000 ns -
IDbCommand cmd;
if (CreateCommand == null)
    cmd = cnn.CreateCommand();
else
    cmd = CreateCommand(cnn);

@billrob
Copy link
Contributor

billrob commented Oct 31, 2024

@RyanThomas73 I went with a new constructor overload to minimize impact across the optional parameters.

#2128

@RyanThomas73
Copy link

RyanThomas73 I went with a new constructor overload to minimize impact across the optional parameters.

@billrob Awesome! Your PR definitely solves my use cases.

Side Note: Not sure if the dapper maintainers have any interested in this approach - I have a Roslyn analyzer + code fix provider you can see here that flags the use of optional parameters and recommends/fixes them to use explicit argumentName: value syntax. In case that would help minimize internal impact of the use of optional parameters. Obviously, doesn't help with consumers using optional parameters that are part of the public API surface area though.

@RyanThomas73
Copy link

Also as note, you put an if check and directly invoke. The performance different is small but still is an order of magnitude slower.

@billrob If it's not a huge burden could you share a gist of your microbenchmark for this?

@billrob
Copy link
Contributor

billrob commented Oct 31, 2024

https://gist.github.com/billrob/98c07d60a4b424a06b6bc2690b623044

@michaelgayeski
Copy link

michaelgayeski commented Jan 7, 2025

To others that want to do this you can create a proxy IDbConnection then run Dapper against that:

	private class RetryLogicProviderProxy(SqlConnection conn, SqlRetryLogicBaseProvider retryLogicProvider ) : IDbConnection
	{
		public void Open() => conn.Open();

		[AllowNull]
		public string ConnectionString
		{
			get => conn.ConnectionString;
			set => conn.ConnectionString = value;
		}

		public int ConnectionTimeout => conn.ConnectionTimeout;

		public string Database => conn.Database;

		public ConnectionState State => conn.State;
		public IDbCommand CreateCommand()
		{
			var cmd = conn.CreateCommand();
			cmd.RetryLogicProvider = retryLogicProvider;

			return cmd;
		}
		
		// ..snip..

	}

Usage:

	var retryLogicProvider = SqlConfigurableRetryFactory.CreateExponentialRetryProvider(new SqlRetryLogicOption()
	{
		MinTimeInterval = TimeSpan.FromSeconds(2),
		MaxTimeInterval = TimeSpan.FromSeconds(10),
		DeltaTime = TimeSpan.FromSeconds(2),
		NumberOfTries = 5,
		TransientErrors = [208] // Table not found
	});

	using SqlConnection inner = new SqlConnection(connectionString) { RetryLogicProvider = retryLogicProvider };
	using IDbConnection db = new RetryLogicProviderCommandProxy(inner, retryLogicProvider); // Could use a different provider for commands.
        db.Open();
																									// 
	const string sql = "SELECT * FROM [DoesNotExist]";
	db.ExecuteReader(sql); // Will retry

@RyanThomas73
Copy link

RyanThomas73 commented Jan 8, 2025

To others that want to do this you can create a proxy IDbConnection then run Dapper against that:

I experimented with doing this and I ran into issues that made me think SqlConnection has internal logic tied to it's specific type that doing this will break.

e.g. Try using the above while enlisting in an ambient transaction via transaction scope.

using (var transcationScope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled))
{
    .... proxied sql connection code ...
}

Additional Notes: I think I was using DbConnection and async signatures as well with the issues I saw compared to your IDbConnection and db.Open() example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants