foreach loop that sets property correctly but appears wrong later

Question

I have a query to the database that returns results and I have an IEnumerable that contains another IEnumerable and in the Model Populator I do something like

List<Parent> parents = Result1;
List<Children> children = Result2;

And then

foreach (parent p in parents)
{
 p.MyChildren = children.Where(x => x.ParentId == p.Id);
}

I have debugged so far and the data are correct by the time I return the model both during the loop and also at the final model which contains the models.

However, on the controller I get a list of parents and they ALL have the same children collection, even though they were correct a moment before. That means each parent has the same random collection in "MyChildren" and not the collection that belongs to them.

My view models have no static variables anywhere and there is no other object manipulation going on from the model to the controller.

Something really odd going on with the references and I am not sure how to resolve it. Any ideas?

Can you provide a little more code? Your question suggests a scope problem, and there's not enough code here to determine scope. — Robert Harvey, Dec 06 '12 at 18:32
I suspect we are missing something here. For one, `parent.MyChildren` should be `p.MyChildren`. What you have there wouldn't even compile as-is. — Eric Petroelje, Dec 06 '12 at 18:33
seems it should be `p.MyChildren = ...`, unless `MyChildren` is a static field/property of `parent` type — Igor, Dec 06 '12 at 18:33
Sorry the p.MyChildren was my bad.. I didn't use the real variable names — Nick, Dec 07 '12 at 10:32

score 3 · Accepted Answer · edited May 23 '17 at 11:55

3

Try adding ToList() to the right hand side, when setting the children:

foreach (parent p in parents)
{
    var id = p.Id;
    parent.MyChildren = children.Where(x => x.ParentId == id).ToArray();
}

The result of Where is an IEnumerable, and the actual retrieval of the items is deferred. That means if something changes later, parent.MyChildren changes. Adding ToArray() forces the enumeration to happen immediately.

Update per Servy's comment

You also have to create a local (inside the foreach block) copy of p.Id (updated above). This problem is called "accessing a modified closure" More info: for example. See also here for more background, including an answer from Eric Lippert where he describes modified closures as one of the worst "gotchas" in C#.

edited May 23 '17 at 11:55

Community

1
1

answered Dec 06 '12 at 19:08

McGarnagle

101,349
31
229
260

1

You're missing the fact that he's closing over the loop variable. That, in combination with deferred execution, is what's causing the problem. – Servy Dec 06 '12 at 19:22
I'll be damned. That did the trick!!! I am still confused how this is acceptable C# behaviour. As I mentioned the debugger shows that the loop does the right thing when the object is returned. It's immediately aftewards in the controller that the data are mangled... – Nick Dec 07 '12 at 10:42
@Nick yes, it's really something you have to watch out for when working with enumerables. And I think it's *not* acceptable behavior -- if you check Lippert's answer at the link, you'll see he says they're going to make a breaking change, in the next version, to fix it: http://stackoverflow.com/q/8898925/1001985 – McGarnagle Dec 07 '12 at 19:19

foreach loop that sets property correctly but appears wrong later

1 Answers1