Support SaveChanges while iterating a query

[rowanmiller]

using (var db = new UnicornStoreContext())
{
    foreach (var p in db.Products)
    {
        db.Products.Add(new Product { DisplayName = "Copy of " + p.DisplayName });
        db.SaveChanges();
    }
}

SqlException: New transaction is not allowed because there are other threads running in the session.
System.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)

Obviously this is a bit of an anti-pattern since the SaveChanges should go outside the loop, but this is just a boiled down repro from a case where it does make sense (calling another component like Idenitty that saves to the database as part of the API call).

foreach (var role in db.PreApprovals
    .Where(p => p.UserEmail == user.Email)
    .Select(p => p.Role))
{
    await UserManager.AddToRoleAsync(user, role);
}

Model from the repro...

    public class UnicornStoreContext : DbContext
    {
        public DbSet<Product> Products { get; set; }

        protected override void OnConfiguring(DbContextOptions options)
        {
            options.UseSqlServer("Server=(localdb)\\v11.0;Database=UnicornStore;Trusted_Connection=True;MultipleActiveResultSets=true");
        }
    }

    public class Product
    {
        public int ProductId { get; set; }
        public string DisplayName { get; set; }
        public string Description { get; set; }
        public decimal MSRP { get; set; }
        public decimal CurrentPrice { get; set; }
    }

[divega]

FWIW, you would get the same on EF6 and the source is a limitation of SQL Server/SqlClient. In the past we answered this was by design and the workaround was add ToList() to the query. True that the Identity example is more interesting because it is more opaque that you are actually sharing a connection with Identity. Buffering queries could be the cure.

[rowanmiller]

Yeah I didn't realize we had the same thing on EF6.

I think this is much more likely to be hit now with the baked in DI patterns in ASP.NET 5. Typically in EF6 days the Identity services would have been using a different instance of the context than the controller... but now it is very easy to end up with two pieces of code using the same context instance within a request without that being immediately obvious from the code you are writing (as was the case for me).

[ajcvickers]

Closing as we haven't seen significant demand for this feature. We could revisit this decision based on feedback and we would also consider a well-written community PR for it.

[dazbradbury]

@ajcvickers - Is this definitely a "wont-fix"? Just one of the issues we came across when migrating to EF Core 3.1.5.

The argument for not materialising a big query hopefully pretty obvious though. Imagine a scenario where the list is quite long, and the reason we don't want to call saveChanges() outside the loop is because there is external work (eg. sending of emails) reliant on the state being saved. This is a reasonably common scenario.

The workaround of materialising all objects is fine, but it can have a memory burden and you need to know about it up-front (not intuitive).

The even less intuitive workaround is to chunk the requests, materialise a batch, do the work etc. This solves the memory burden and EF core issue, but is definitely not intuitive / what a dev would write off the bat.

Would love for this to be solved at some point to avoid "gotchas" when working with loop + update style code - even if I appreciate there are workarounds.

[AndriySvyryd]

@dazbradbury Perhaps you can use a factory to create a new context for the nested SaveChanges

[dazbradbury]

Thanks @AndriySvyryd - that's another approach, although depending on what's happening in the loop and after the loop this is likely to give rise to change conflicts without further thought. So again, not super intuitive.

Our team actually created an analyser to catch these instances so devs don't fall into the trap and can use one of the strategies to workaround it. I can't imagine the intention of the EF core team is for teams to write analysers though, which is why I wanted to raise it as an ongoing issue rather than a "closed"/"solved" one in the hope @ajcvickers would revisit that decision.

[ajcvickers]

@dazbradbury I read through to understand again. I don't think it's something we are planning to revisit.

[dazbradbury]

Understood. We're happy to rely on the analyser, I guess if anyone else runs into this they can comment / follow this issue. Thanks for letting us know in any case so we can plan around it.