Efcore: Document that modifying entity states while iterating over entries can result in "Collection was modified" exception

[bricelam]

System.InvalidOperationException : Collection was modified; enumeration operation may not execute.
   at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource)
   at System.Collections.Generic.Dictionary`2.ValueCollection.Enumerator.MoveNext()
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityReferenceMap.<GetEntriesForState>d__12.MoveNext() in src\EFCore\ChangeTracking\Internal\EntityReferenceMap.cs:line 363
   at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext()
   at Microsoft.EntityFrameworkCore.GraphUpdatesTestBase`1.<Detaching_dependent_entity_will_remove_references_to_it>b__107_0(DbContext context) in test\EFCore.Specification.Tests\GraphUpdatesTestBase.cs:line 919
   at Microsoft.EntityFrameworkCore.TestUtilities.TestHelpers.<>c__DisplayClass39_0`1.<ExecuteWithStrategyInTransaction>b__0(TContext context) in test\EFCore.Specification.Tests\TestUtilities\TestHelpers.cs:line 429
   at Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.<>c__2`1.<Execute>b__2_0(<>f__AnonymousType0`2 s) in src\EFCore\Storage\ExecutionStrategyExtensions.cs:line 73
   at Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.<>c__DisplayClass12_0`2.<Execute>b__0(DbContext c, TState s) in src\EFCore\Storage\ExecutionStrategyExtensions.cs:line 338
   at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.ExecuteImplementation[TState,TResult](Func`3 operation, Func`3 verifySucceeded, TState state) in src\EFCore\Storage\ExecutionStrategy.cs:line 195
   at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded) in src\EFCore\Storage\ExecutionStrategy.cs:line 159
   at Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.Execute[TState,TResult](IExecutionStrategy strategy, Func`2 operation, Func`2 verifySucceeded, TState state) in src\EFCore\Storage\ExecutionStrategyExtensions.cs:line 336
   at Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.Execute[TState,TResult](IExecutionStrategy strategy, TState state, Func`2 operation) in src\EFCore\Storage\ExecutionStrategyExtensions.cs:line 286
   at Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.Execute[TState](IExecutionStrategy strategy, TState state, Action`1 operation) in src\EFCore\Storage\ExecutionStrategyExtensions.cs:line 70
   at Microsoft.EntityFrameworkCore.TestUtilities.TestHelpers.ExecuteWithStrategyInTransaction[TContext](Func`1 createContext, Action`2 useTransaction, Action`1 testOperation, Action`1 nestedTestOperation1, Action`1 nestedTestOperation2, Action`1 nestedTestOperation3) in test\EFCore.Specification.Tests\TestUtilities\TestHelpers.cs:line 421
   at Microsoft.EntityFrameworkCore.GraphUpdatesTestBase`1.ExecuteWithStrategyInTransaction(Action`1 testOperation, Action`1 nestedTestOperation1, Action`1 nestedTestOperation2, Action`1 nestedTestOperation3) in test\EFCore.Specification.Tests\GraphUpdatesFixtureBase.cs:line 2782
   at Microsoft.EntityFrameworkCore.GraphUpdatesTestBase`1.Detaching_dependent_entity_will_remove_references_to_it() 

Use feature/net472 branch to investigate

[jamesport079]

Was scratching my head with this error. Then I went back to EF Core 2.2 and error was not there any more. So it must be something to do with EF Core 3.0. I thought I'm doing something wrong on my end. I'm getting this error when using ReloadAsync, but the error is not that easy to reproduce.

        foreach (var entry in Context.ChangeTracker.Entries<LabWork>())
        {
            await entry.ReloadAsync().ConfigureAwait(true);
        }

[roji]

This happens because of the behavior introduced in 7e65b82821ce2540d12d35c6449712f07c56a527, which the failing test exercises (removing references to an entity when it is detached. Detaching_dependent_entity_will_remove_references_to_it iterates over the change tracker entries. It then sets all entries to state detached, which internally removes them from the entity reference map for their old state (_unchangedReferenceMap in this case). So we have EntityReferenceMap.GetEntriesForState iterating over _unchangedReferenceMap on the one hand, and EntityReferenceMap.Remove modifying the same dictionary.

The somewhat mysterious thing is that .NET Framework fails on this while .NET Core doesn't - but the dictionary implementation has likely changed so much that it's not implausible. I've tried to put together a quick minimal repro of remove-while-iterating on Dictionary which works on Core and fails on Framework, but it's more subtle than a 2 minute effort. In any case, the exception is the expected behavior here, and it could happen in .NET Core as well.

We could fix this simply by adding ToList in the test. However, the possibly problematic thing is that it's not obvious that setting an entry state change modifies the change tracker in a way that would trigger this exception (i.e. I'd expect people to fall into this pit a lot). We could switch to ConcurrentDictionary but that'd probably be bad for perf, any thoughts @ajcvickers?

@jamesport079 you should be able to at least work around your ReloadAsync issue by putting ToList after Entries<LabWork>, allowing you to continue being in 3.0.

[jamesport079]

Thanks! Worked without problems.

[ajcvickers]

@roji I reverted that change a couple of days ago in #18515. Does this mean we should no longer hit this issue, or could it still occur in other cases?

[roji]

@ajcvickers I'll boot to Windows later for a specific test, but as far as I can tell the issue is more general than that; it also wasn't really introduced by your previous commit. Basically any code which changes an entry's state while iterating over entries could trip this, since the state setter ultimately calls into StateMananger.StateChanging, which removes the entry from the old state's reference map and adds it to the new.

I'm not convinced there's an actual problem here - we could say that changing a state's entity obviously modifies the state manager, and so you can't do it while iterating over the latter. I'm just worried it's not completely obvious, plus it doesn't seem to bomb currently (with the cases we've seen), but may start bombing as Dictionary changes.

But keep in mind I'm pretty new to the tracking side, so maybe it's OK to expect users to understand the mutation/iteration thing.

[ajcvickers]

@roji We usually make defensive copies for situations like this to allow iteration and modification. It sucks for perf, but it keeps things working.

[roji]

We usually make defensive copies for situations like this

Sure, although here it's the users who need to be aware that they need to do defensive copies, and there's some evidence that this isn't self-evident.

On the other hand I don't have any other suggestion aside from documentation. I can just submit a PR to fix the specific test (add ToList), let me know.

[jamesport079]

Wasn't self evident in my case :) Running 3.1 preview 1 not the nightly build, so can't tell if issue was fixed after #18515.

[ajcvickers]

@roji What I meant was defensive copies _in the EF code_. For example, here: https://github.com/aspnet/EntityFrameworkCore/blob/release/3.1/src/EFCore/ChangeTracking/ChangeTracker.cs#L163

Obviously there are perf implications of this, so we should consider other options as well, but at least historically we would fix bugs like this in EF, rather than expecting users to do the defensive copy themselves.

[roji]

Understood. It's certainly easy to do that, meaning that it would be effectively impossible to enumerate anything entries in the change tracker, only get a snapshot, which seems like a pretty heavy hammer. I guess the primary question is whether we think there are scenarios for inspecting tracking entries which do not also involve their modification; I don't have enough experience here but intuitively I'd avoid forcing expensive snapshotting on everyone if at least some scenarios justify enumeration without modification of the entries.

Although it's a potential pit of failure, it doesn't seem completely unreasonable to ask users to view as the change tracker much like a Dictionary: if you iterate anything in it, you can't modify any entries while you're doing it.

[jamesport079]

Makes sense to snapshot in my case.

[ajcvickers]

@roji I agree that the perf impact of doing a snapshot every time is likely too much--this is an area that can be sensitive to perf. Let's discuss in triage.

[ajcvickers]

@roji Although it just occurred to me that we should understand why this doesn't fail on .NET Core. It's not a threading issue like we first thought, so did they change something in the implementation of the underlying collections that means it's okay to modify in this case, even when enumerating? If so, it might not be worth doing anything just for .NET Framework.

[roji]

@ajcvickers yeah, the implementation for Dictionary has changed a lot in .NET Core. Even in .NET Framework removing an element while iterating doesn't necessarily trigger an exception - it depends on how things happen to be arranged internally etc. I just don't know what can exactly can be expected, but I'm pretty sure nothing is guaranteed.

I'm frankly a bit surprised there's not a 100% reliable guard in there against mutation during iteration - it means things could work well in some scenarios (and during testing) and fail in production. We could reach out to the relevant people and ask for more info.

[roji]

It turns out that since .NET Core 3.0, Dictionary officially supports removing elements during enumeration (https://github.com/dotnet/coreclr/pull/18854).

Other mutation is not supported, and enumeration will very deterministically fail immediately when enumeration is attempted to continue after insertion. Now, when the change tracker is enumerated, internally the different entity reference maps are enumerated one after the other (added, modified, deleted...). The test in this issue always removes the entry from the reference map currently being enumerated, and inserts it into another map (unchanged), so there's no insertion during enumeration.

However, it is still possible to get an exception by inserting into the same reference map being enumerated. For example:

c# ctx.Blogs.Add(new Blog()); foreach (var blogs in ctx.ChangeTracker.Entries<Blog>()) ctx.Blogs.Add(new Blog());

[ajcvickers]

Notes from triage:

• Making the default safe would negatively impact perf for common read-only scenarios
• There was pushback on adding a discoverable API to choose the behavior as not adding much value
• So the decision is to document this for now. However, we may revisit based on feedback from other people hitting this issue.