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

ExecutionStrategy.ShouldRetryOn receives a null argument with concurrency exceptions #23019

Closed
ixtreon opened this issue Oct 16, 2020 · 0 comments · Fixed by #25836
Closed

ExecutionStrategy.ShouldRetryOn receives a null argument with concurrency exceptions #23019

ixtreon opened this issue Oct 16, 2020 · 0 comments · Fixed by #25836
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@ixtreon
Copy link

ixtreon commented Oct 16, 2020

The ExecutionStrategy base class automatically unwraps EF exceptions before passing them to the ShouldRetryOn method. The method which does the unwrapping, CallOnWrappedException, seems to directly recurse on the InnerException property of the parent exception without checking whether it is non-null.

This is not an issue if all instances of DbUpdateException have a non-null InnerException, but it seems that DbUpdateConcurrencyException exceptions thrown by the SQL Server and Sqlite providers don't include one. This causes such exceptions to be undetectable from the ShouldRetryOn method, when deriving from the ExecutionStrategy base class.

As this violates the [NotNull] attribute from the method's signature, this looks like a bug either with that method, or with the DbUpdateConcurrencyException type which doesn't have an InnerException.

Code to Reproduce
// DB Model
class User
{
    public int ID { get; set; }

    [ConcurrencyCheck]
    public string Email { get; set; }
}

class MyDbContext : DbContext
{
    public DbSet<User> Users { get; set; }

    public MyDbContext(DbContextOptions options)
        : base(options)
    {

    }
}

// Custom ExecutionStrategy
class CustomExecutionStrategy : ExecutionStrategy
{
    public CustomExecutionStrategy(DbContext context, int maxRetryCount, TimeSpan maxRetryDelay)
        : base(context, maxRetryCount, maxRetryDelay)
    { }

    protected override bool ShouldRetryOn(Exception exception)
    {
        if (exception == null)
            throw new Exception("Unexpected null exception");

        return exception is DbUpdateConcurrencyException;
    }
}

// NUnit test class

[TestFixture]
class ExecutionStrategyTests2
{

    DbContextOptions dbOptions;

    [SetUp]
    public void init()
    {
        // reuse same connection & db options as we want a single in-memory DB
        var connection = new SqliteConnection("DataSource=:memory:");
        connection.Open();

        dbOptions = new DbContextOptionsBuilder()
            .UseSqlite(connection)
            .UseLoggerFactory(LoggerFactory.Create(builder => builder.AddQueryDebug()))
            .EnableSensitiveDataLogging(true)
            .Options;

        // init DB and insert a single user
        var tempContext = new MyDbContext(dbOptions);
        tempContext.Database.EnsureCreated();

        tempContext.Set<User>().Add(new User
        {
            Email = "test@example.com",
        });
        tempContext.SaveChanges();
    }

    [Test]
    public void CustomExecutionStrategy_CatchesConcurrencyException()
    {
        var contextA = new MyDbContext(dbOptions);
        var strategy = new CustomExecutionStrategy(contextA, 3, TimeSpan.FromSeconds(5));
        var attemptsMade = 0;

        strategy.Execute(() =>
        {
            var entity = contextA.Set<User>().Single();

            // simulate a concurrency exception on first attempt only
            if (attemptsMade == 0)
            {
                var contextB = new MyDbContext(dbOptions);
                contextB.Set<User>().Single().Email = "second@example.com";
                contextB.SaveChanges();
            }
            attemptsMade++;

            entity.Email = "first@example.com";
            contextA.SaveChanges();
        });

        var tempContext = new MyDbContext(dbOptions);
        var entity5 = tempContext.Set<User>().Single();
    }
}
Results
Message: 
    System.Exception : Unexpected null exception
Stack Trace: 
    CustomExecutionStrategy.ShouldRetryOn(Exception exception) line 52
    ExecutionStrategy.ExecuteImplementation[TState,TResult](Func`3 operation, Func`3 verifySucceeded, TState state)
    ExecutionStrategyExtensions.Execute(IExecutionStrategy strategy, Action operation)
    ExecutionStrategyTests2.CustomExecutionStrategy_CatchesConcurrencyException() line 95

EF Core version: 3.1.9
Database provider: Microsoft.EntityFrameworkCore.SqlServer, Microsoft.EntityFrameworkCore.Sqlite
Target framework: .NET 4.6.1
Operating system: Windows 10
IDE: Visual Studio 2019 16.3.7

@AndriySvyryd AndriySvyryd self-assigned this Oct 16, 2020
@AndriySvyryd AndriySvyryd added this to the 6.0.0 milestone Oct 16, 2020
@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 Sep 2, 2021
@AndriySvyryd AndriySvyryd removed their assignment Sep 2, 2021
AndriySvyryd added a commit that referenced this issue Sep 2, 2021
Don't pass null to ExecutionStrategy.ShouldRetryOn.
Use the currently executing execution strategy to determine whether buffering is needed.

Fixes #21350
Fixes #23019
Fixes #21463
AndriySvyryd added a commit that referenced this issue Sep 2, 2021
Don't pass null to ExecutionStrategy.ShouldRetryOn.
Use the currently executing execution strategy to determine whether buffering is needed.

Fixes #21350
Fixes #23019
Fixes #21463
AndriySvyryd added a commit that referenced this issue Sep 2, 2021
Don't pass null to ExecutionStrategy.ShouldRetryOn.
Use the currently executing execution strategy to determine whether buffering is needed.

Fixes #21350
Fixes #23019
Fixes #21463
AndriySvyryd added a commit that referenced this issue Sep 3, 2021
Don't pass null to ExecutionStrategy.ShouldRetryOn.
Use the currently executing strategy to determine whether buffering is needed.

Fixes #21350
Fixes #23019
Fixes #21463
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-rc2 Sep 4, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-rc2, 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. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants