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

Need easier options to suspend / bypass the configured DbContext's IExecutionStrategy #24922

Closed
simeyla opened this issue May 18, 2021 · 5 comments
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. community-contribution customer-reported good first issue This issue should be relatively straightforward to fix. type-enhancement
Milestone

Comments

@simeyla
Copy link

simeyla commented May 18, 2021

I am currently using the following execution strategy, the 'set it and forget it' approach. It usually works great!

 options.UseSqlServer(connection, sqlOptions =>
 {
     sqlOptions.EnableRetryOnFailure();
 });

I need to be able to suspend / replace the configured IExecutionStrategy for certain cases where I need to use an explicit transaction.

A solution therefore is needed to avoid the following exception:

InvalidOperationException: The configured execution strategy 'SqlServerRetryingExecutionStrategy' does not support user initiated transactions. Use the execution strategy returned by 'DbContext.Database.CreateExecutionStrategy()' to execute all the operations in the transaction as a retriable unit.

The exception is described in Connection Resiliancy as being a problem if you're using an explicit BeginTransaction. The solution provided is to use this line of code db.Database.CreateExecutionStrategy().Execute(() => ... ) instead of an explicit or ambient transaction.

This solution can't be used if you need two active transactions for operations in two different databases. Here's a simplification of what I am logically trying to achieve:

  • Perform an operation in one database.
  • Perform an operation in a second database (not a different context).
  • Roll them both back if they don't both succeed.

Since we don't have distributed transactions yet in EF Core, I wanted to create two transaction scopes and then commit them together. The following code will work, but only if you do not have a retrying execution strategy defined.

// The contexts represent TWO DIFFERENT databases on Azure
// Everything with a 1 is the first database
// Everything with a 2 is the second database
using (var tran1 = _context1.Database.BeginTransaction())
{
    using (var tran2 = _context2.Database.BeginTransaction())
    {
        // Create a thing1 and insert it into the table in DB1
        var thing1 = new Thing1();
        _context1.Thing1s.Add(thing1);

        // Save it to get the DB generated identity key 
        await _context1.SaveChangesAsync();

        // Now create a thing2 which requires the key for the thing1 we just inserted
        var thing2 = new Thing2()
        {
            thing1ID = thing1.ID
        };

        // Insert into DB 2 and save changes
        _context2.Thing2s.Add(thing2);
        await _context2.SaveChangesAsync();

        // commit both if everything worked
        await tran1.CommitAsync();
        await tran2.CommitAsync();
    }
}

For my needs this is an acceptable low-risk alternative to fully distributed transactions. I lose the auto-retry but I don't see any other alternative at this time.

The recommended solution (db.Database.CreateExecutionStrategy().Execute(() => ... )) unfortunately does not help in this situation since both transaction scopes need to remain active until they are committed.

I've looked through some of the source to try to find solutions. Here are my attempts and the limitations I came across:

Attempt 1: Use NonRetryingExecutionStrategy

I found NonRetryingExecutionStrategy which does exactly what I want, however there is no constructor taking a DbContext. The constructor takes ExecutionStrategyDependencies which is basically impossible for me to construct in my own code.

Suggestion 1: Add a new constructor to NonRetryingExecutionStrategy

This would allow me to do this in the scope of a preexisting transaction.

await new NonRetryingExecutionStrategy(_context1).ExecuteAsync(() => ...)

Attempt 2: Use SqlServerRetryingExecutionStrategy with 0 retries

Next I attempted to do the following:

await new SqlServerRetryingExecutionStrategy(_context, 0).ExecuteAsync(() => ...)

This should work since it's no longer retrying anything (and that's the whole reason the exception is thrown).

This unfortunately does not work since the OnFirstExecution method in ExecutionStrategy (which is the base class) will ALWAYS throw the exception if there is an active transaction.

Suggestion 2: Do not throw the exception if retries are disabled (RetriesOnFailure == false)

In other words update ExecutionStrategy.OnFirstExecution to the following:

protected virtual void OnFirstExecution()
{
    if (RetriesOnFailure == false) 
    { 
        // do not throw exception
    }
    else { 
        // existing logic
    }
 } 

Then RetriesOnFailure would need to consider the MaxRetryCount

public virtual bool RetriesOnFailure
        => !Suspended && MaxRetryCount > 0;

Attempt 3: Create my own custom execution strategy

This succeeded! My implementation is minimal, but with the following override.

    protected override void OnFirstExecution()
    {
         // do nothing!!
    }

While this worked it quite frankly took me all day to first be reminded (and annoyed!) that distributed transactions aren't yet supported, and then fail to find an existing solution.

PS: I'm pretty sure this succeeded in my brief testing! If there's some reason why this wouldn't work I'd love to hear it :-)

Attempt 4: Make other ways to create an execution strategy on the fly

The most discoverable way would be to add another way to create an execution strategy on the DatabaseFacade.

Perhaps DbContext.Database.CreateNonRetryingExecutionStrategy() ?

This could then be documented and it would be completely obvious what it was.

Conclusions

It was simply too hard to disable the configured execution strategy and there needs to be an easier way.
My attempted solutions are not mutually exclusive.

Even with the best solution I came up with, I still need to wrap every group of database operation in ExecuteAsync. If there were an alternative way to disable the configured strategy for the whole context that would be preferable.

Disclaimer:

Clearly things are always more complicated so I may have completely missed something.

@simeyla simeyla changed the title Need easier options to suspend configured retry execution strategy for special cases Need easier options to suspend / bypass the configured DbContext's IExecutionStrategy May 18, 2021
@roji
Copy link
Member

roji commented May 18, 2021

As you wrote, the code fragment you showed above (with the two explicit Database.BeginTransaction) isn't really safe - the first commit could succeed and the second could fail. If that's acceptable, then maybe it's also OK to simply do the two operations one after another:

await using (var ctx1 = new MyContext1())
{
    ctx1.Thing1s.Add(thing1);
    await ctx1.SaveChangesAsync();
}

await using (var ctx2 = new MyContext1())
{
    var thing2 = new Thing2
    {
        thing1ID = thing1.ID
    };

    ctx2.Thing2s.Add(thing2);
    await ctx2.SaveChangesAsync();
}

This has the advantage of retrying for the first operation. It's true that this is slightly more risky than your code sample above (with the two manual BeginTransctions) - since the second INSERT happens after the first COMMIT, as opposed to before in your code sample. However, that seems pretty negligible/theoretical to me, assuming you're willing to accept the risk that's already there anyway.

Another option, is to specify whether you want resiliency when instantiating your DbContext, something like the following:

public class MyContext1 : DbContext
{
    private bool _enableRetryOnFailure;

    public MyContext1()
        => _enableRetryOnFailure = true;

    public MyContext1(bool enableRetryOnFailure)
        => _enableRetryOnFailure = enableRetryOnFailure;

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer("...",
                o =>
                {
                    if (_enableRetryOnFailure)
                        o.EnableRetryOnFailure();
                });
}

This would allow you to selectively disable resiliency when you know you don't want it.

Otherwise, yeah, you can simply extend SqlServerRetryingExecutionStrategy and override OnFirstExecution to not check for a transaction - that should be very trivial to do.

Since we don't have distributed transactions yet in EF Core

Note that EF Core has nothing to do with this - distributed transactions aren't supported by .NET Core, not EF Core.

@stevendarby
Copy link
Contributor

stevendarby commented May 18, 2021

Hi @roji, not OP but also interested in this. Regarding selectively enabling the retry policy at in the config, what’s a good approach for that when using DI? i.e where typically you use AddDbContext once and can’t really control what happens for each instance you request via injection and it’s too late to configure it further once you get the instance.

Would one approach be to create another DbContext class that inherits your base one, register that separately in DI with the different options and then inject the one you need as appropriate? Is there an alternative approach? Having a simple toggle on the context would be really nice but looking for the simplest alternatives that work now. Thanks!

@simeyla
Copy link
Author

simeyla commented May 18, 2021

@roji thanks for your reply.

My actual needs are of course a lot more complicated than this example. It wouldn't work to run the two operations separately, I need to be able to roll them both back if there's an error - typically in the SubmitChanges() call. Maybe it's a timeout, a FK constraint error or a data truncation error. Or maybe I've been careless and get a NullReferenceException somewhere else in my method. I want to use the transaction to protect myself.

First the good news. I found I was able to do the following in .NET 5 without two explicit BeginTransactions, and commit them both together. I still needed to execute each set of operations inside a custom execution strategy (since I have a global strategy) which is a pain but manageable.

var context1Strategy = new CustomExecutionStrategy(_context1);  
var context2Strategy = new CustomExecutionStrategy(_context2);

using (var tran = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled))
{
   await context1Strategy.ExecuteAsync(async () =>{ ... })
   await context2Strategy.ExecuteAsync(async () =>{ ... })
   tran.Complete();
}

I'll admit I'm still slightly confused sometimes exactly what a distributed transaction is. I thought that communicating with two azure databases would qualify as one but it appears it does not. I'll need to read this https://docs.microsoft.com/en-us/azure/azure-sql/database/elastic-transactions-overview :-)

I don't want to lose sight of my original suggestions that could be made either to the framework or documentation. It needs to be easier to opt-out of a configured strategy when a transaction is needed. I reported this as an issue on the repo and not on Stackoverflow for a reason. My goal is to make this easier and less confusing for others in the future!

Otherwise, yeah, you can simply extend SqlServerRetryingExecutionStrategy and override OnFirstExecution to not check for a transaction - that should be very trivial to do.

I'm still thinking over what might be the impact of this. May be an option for me, but clearly not a solution for everyone.

Would one approach be to create another DbContext class that inherits your base one, register that separately in DI with the different options and then inject the one you need as appropriate? Is there an alternative approach? Having a simple toggle on the context would be really nice but looking for the simplest alternatives that work now. Thanks!

@stevendarby I wondered about doing this when I first had this problem. I couldn't see an easy way to do this - I think you'd have to come up with your whole own way of creating a context. For the problem at hand it would be massively overcomplicated.

@AndriySvyryd
Copy link
Member

@simeyla NonRetryingExecutionStrategy is meant to be very lightweight and isn't fit for your scenario since it won't prevent nested execution strategies from throwing.

We can implement suggestion 2, as it's in-line with the current design.

@AndriySvyryd AndriySvyryd added this to the Backlog milestone May 18, 2021
@AndriySvyryd AndriySvyryd added the good first issue This issue should be relatively straightforward to fix. label May 18, 2021
@roji
Copy link
Member

roji commented May 18, 2021

@simeyla a proper distribution transaction (not supported in .NET Core) ensures that both commits either succeed or fail on both databases, never committing one one database but not on the other. Without it, your first commit may succeed on your first database, but the second may fail, in which case you're left with an inconsistent state across your two databases. You can try to protect against this by manually undoing the effects of the first transaction, but... that could fail... This is basically what distributed transactions were created to solve.

martincostello added a commit to martincostello/efcore that referenced this issue Jun 5, 2021
Do not throw an exception on the first execution of an execution
strategy if a transaction is pending when MaxRetryCount is zero.
Addresses dotnet#24922.
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 9, 2021
@AndriySvyryd AndriySvyryd modified the milestones: Backlog, 6.0.0 Jun 9, 2021
@AndriySvyryd AndriySvyryd modified the milestones: 6.0.0, 6.0.0-preview6 Jul 2, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-preview6, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. community-contribution customer-reported good first issue This issue should be relatively straightforward to fix. type-enhancement
Projects
None yet
Development

No branches or pull requests

5 participants