Runtime: Adding IEnumerable to itself with AddRange results in infinite loop

[joshbartley]

If you have an List, and you use .Where() to filter that list into itself using AddRange, it will loop until the system runs out of memory.

.Net 4.6.1 Behavior "Collection was modified enumeration operation may not execute"
.Net Core 2.1 Behavior OutOfMemoryException

The List has behavior currently for checking if an ICollection is the same reference, the Else fall through does not have this check. Also the Enumerator is being modified but is not throwing an exception, wanted to start a conversation about this since there could be two possible fixes, or this could be new expected behavior different from .Net 4.6.1 or earlier.

Quick sample repo for both 4.6.1 and .Net Core 2.1.
c# class Program { static void Main(string[] args) { var testList = new List<TestObject>(); testList.Add(new TestObject() { Property1 = "test" }); testList.AddRange(testList.Where(x => x.Property1 == "test")); } } public class TestObject { public string Property1 { get; set; } }

[karelz]

Does it reproduce if you take Linq out of the picture? Or does the repro depend on Linq (.Where) being used?

[joshbartley]

Removing the .Where does not cause any exception. Also adding a .ToList in the statement also does not cause any exceptions to generate. It just seems to only occur if it is an IEnumerable.

[Clockwork-Muse]

What happens if you add items that _don't_ match the Where clause - before or after the matching one? What about multiple matching items?

[joshbartley]

Same result, The original code that ran into this issue had ~100 records in it but only 1 matched the Where clause, and this was a quick way to additional data without copy/pasting an object initializer.

[karelz]

Simpler repro: (tried on .NET 4.7.1 vs. .NET Core 2.1)
```c#
using System.Collections.Generic;
using System.Linq;

class Program
{
static void Main()
{
var testList = new List() { 1 };
testList.AddRange(testList.Where(_ => true));
}
}
```

[jhudsoncedaron]

I have to admit I feel this shouldn't be fixed but who am I to know.

In any case I think josbartley's code should be doing .First(predicate) rather than .Where(predicate) to obviously get the behavior he wants.

[jnm2]

Mutating the source collection while enumerating really should be causing 'Collection was modified enumeration operation may not execute' on all platforms. That's the appropriate fix, I'm pretty sure.

[stephentoub]

I expect this was broken by https://github.com/dotnet/coreclr/pull/8306. Previously the AddRange implementation would enumerate, and for each item, would call Add, which would increment the version number, and then the next access to the enumerator would throw. With that change, as noted in https://github.com/dotnet/coreclr/pull/8306#discussion_r89682000, the version increment is removed for each item, and thus the enumerator doesn't know it should be invalidated.

There are no perf numbers included in that PR. My initial reaction is that PR should just be reverted.

cc: @jkotas, @safern, @ianhays, @jamesqo

[safern]

There are no perf numbers included in that PR. My initial reaction is that PR should just be reverted.

From looking at the discussions in the PR and the changes it looks like there wasn't a good/enough investigation on what was changing from that PR. So I agree that I have the same feeling that we should just revert that PR and then do a deeper investigation on why Add wasn't being inlined in the first place.

Also it seems to me like we should've caught this in our tests, so it seems like a test hole here to me.