What's likely happening is that SignalData
is indirectly changing the subscribers dictionary under the hood during the loop and leading to that message. You can verify this by changing
foreach(Subscriber s in subscribers.Values)
To
foreach(Subscriber s in subscribers.Values.ToList())
If I'm right, the problem will disappear.
Calling subscribers.Values.ToList()
copies the values of subscribers.Values
to a separate list at the start of the foreach
. Nothing else has access to this list (it doesn't even have a variable name!), so nothing can modify it inside the loop.
For me this looks like a bug in Entity Framework. I've cooked down your example to a simpler one but with the same structure:
public class TestA // corresponds to your Employee
{
public int Id { get; set; }
public TestB TestB { get; set; } // your Employer
}
public class TestB // your Employer
{
public TestB()
{
TestCs = new List<TestC>();
}
public int Id { get; set; }
public ICollection<TestC> TestCs { get; set; } // your EmployeeRoles
}
public class TestC // your EmployeeRole
{
public int Id { get; set; }
public TestA TestA { get; set; } // your Employee
}
These are three entities with cyclic relationships:
TestA -> TestB -> TestC -> TestA
If I use now a corresponding code with the same structure than yours I get the same exception:
var testA = new TestA();
var testB = new TestB();
var testC = new TestC();
context.TestAs.Add(testA);
testA.TestB = testB;
testB.TestCs.Add(testC);
testC.TestA = testA;
context.ChangeTracker.DetectChanges();
Note that I have used DetectChanges
instead of SaveChanges
because the stack trace in the exception makes clear that actually DetectChanges
causes the exception (which is called internally by SaveChanges
). I also found that calling SaveChanges
twice is not the problem. The problem here is only the "early" adding to the context before the whole object graph is completed.
The collection which was modified (as the exception is complaining about) is not the TestB.TestCs
collection in the model. It seems to be a collection of entries in the ObjectStateManager
. I could verify this by replacing ICollection<TestC> TestCs
with a single reference by TestC TestC
in the TestB
class. This way the model doesn't contain any collection at all but it still throws the same exception about a modified collection. (SaveChanges
will fail though with three single references because EF doesn't know in which order to save the entities due to the cycle. But that is another problem.)
I would consider it as a bug that EF change detection (DetectChanges
) seems to modify its own internal collection it is just iterating through.
Now, the fix to this problem is easy: Just Add
entities to the context as your last step before you call SaveChanges
:
var testA = new TestA();
var testB = new TestB();
var testC = new TestC();
testA.TestB = testB;
testB.TestCs.Add(testC);
testC.TestA = testA;
context.TestAs.Add(testA);
context.ChangeTracker.DetectChanges();
EF will add the whole related object graph to the context. This code succeeds (also using SaveChanges
instead of DetectChanges
).
Or your example:
using (DbContext db = DbContext.GetNewDbContext()){
Employee creator = new Employee("Bob");
Employer employer = new Employer("employer", creator);
db.Employees.Add(creator);
db.SaveChanges();
}
Edit
This was the same exception: Entity Framework throws "Collection was modified" when using observable collection. Following the code in that example the situation was similar: Adding an entity to the context and then afterwards changing/adding relationships to that entity.
Edit2
Interestingly this throws the same exception:
var testA = context.TestAs.Find(1); // assuming there is already one in the DB
var testB = new TestB();
var testC = new TestC();
testA.TestB = testB;
testB.TestCs.Add(testC);
testC.TestA = testA;
context.SaveChanges(); // or DetectChanges, it doesn't matter
So, I want to add relationships with new entities to the existing entity. A fix to this problem seems to be less obvious.
Best Answer
This is a pretty common mistake - modifying a collection whilst iterating it using
foreach
, keep in mind thatforeach
uses readonlyIEnumerator
instance.Try to loop through the collection using
for()
with an extra index check so if the index is out of bounds you would be able to apply additional logic to handle it. You can also use LINQ'sCount()
as another loop exit condition by evaluating theCount
value each time if the underlying enumeration does not implementICollection
:If
Markers
implementsIColletion
- lock on SyncRoot:Use
for()
:You might find this post useful: How do foreach loops work in C#?