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

Comment by Ken McCormack on recover-from-dbupdate-exception #1686

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

haacked
Copy link
Owner

@haacked haacked commented Nov 30, 2023

avatar:

Thanks for the post Phil! I held back a PR on DbContext pooling - because of this concern. I couldn't find any doc on whether EF Core auto-cleans. The symptom first revealed itself in a set of integration tests, which used the same DbContext for a series of fixture creates... (a null string in a non-nullable property will do it), so yes, if the DbContext gets an "unsavable" entity added to its state, all the tests in the suite go red.

(So, would this leak away valid DbContexts in the pool, and cause random failures, slowly bringing the system down??)

So, another callout is that every consumer should not need to handle this... it's an orthogonal concern. Analogous to this are services setting common fields like "CreatedBy" and "CreatedDate" (DRY fail code smell). To get around this, we override SaveChangesAsync in our DbContext, and pass in a 'caller' object, so fields like CreatedBy / UpdatedBy, CreatedDate, UpdatedDate are set centrally (if the type implements a custom interface "IAuditedEntity") so, every repository was doing this, it's now one piece of code - see below -

public virtual async Task SaveChangesAsync(UserId callerId)
{
var now = _dateTimeService.UtcNow; // so that tests are deterministic

// ChangeTracker is null in a unit test - has to be an integ test
foreach (var changedEntity in ChangeTracker.Entries())
{
    if (changedEntity.Entity is IAuditedEntity entity)
    {
        switch (changedEntity.State)
        {
            case EntityState.Added:
                entity.CreatedDate = now;
                entity.UpdatedDate = null;
                entity.CreatedBy = callerId;
                entity.UpdatedBy = null;
                break;
            case EntityState.Modified:
                Entry(entity).Property(x => x.CreatedBy).IsModified = false;
                Entry(entity).Property(x => x.CreatedDate).IsModified = false;
                entity.UpdatedDate = now;
                entity.UpdatedBy = callerId;
                break; 
        }
    }
}

// todo - add Detach in a try-catch here, to clean the pooled context
return await base.SaveChangesAsync();
}

So, puzzle... how to write an integration test to replicate a parallel unit of work performing a breaking insert - it needs to interleave with the main flow's read and insert...

I will try inheriting the DbContext ("TestDbContext"), and adding that into the container, casting it first to the base type. The service should resolve the test context. Its overload for SaveChangesAsync would create a new DbContext (if some test condition is met), and perform an insert, then continue with the service's unit of work... calling base.SaveChangesAsync(). This will throw a DbUpdateException, the DbContext should catch, detach the failing entity, and throw. In the test, calling SaveChangesAsync a second time on the same DbContext instance (adding a new, valid, entity), should work.

(That's a theory!)

@haacked haacked merged commit 538e3b4 into main Nov 30, 2023
@haacked haacked deleted the comment-256acf3b branch November 30, 2023 19:39
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

Successfully merging this pull request may close these issues.

1 participant