ConcurrentBag IndexOutOfRangeException when using ToImmutableArray()

[JakeYallop]

Description

When calling ToImmutableArray() on a ConcurrentBag that also has many concurrent Add calls, an IndexOutOfRangeException is thrown.

Reproduction Steps

using System.Collections.Concurrent;
using System.Collections.Immutable;
using System.Runtime.InteropServices;

ConcurrentBag<string> messages = [];
var cts = new CancellationTokenSource();

PosixSignalRegistration.Create(PosixSignal.SIGINT, ctx =>
{
    ctx.Cancel = true;
    cts.Cancel();
});

var t1 = StartAdding(cts.Token);

while (!cts.Token.IsCancellationRequested)
{
    //All the commented out methods succeed just fine.
    //var b = messages.ToList();
    //var c = messages.ToArray();
    //var d = messages.ToHashSet();
    //var e = messages.ToImmutableList();
    //var f = messages.ToImmutableHashSet();
    var a = messages.ToImmutableArray();
    Console.WriteLine($"{DateTime.UtcNow:ss:fff}");
}

await t1;

async Task StartAdding(CancellationToken cancellationToken)
{
    await Task.Yield();
    while (!cancellationToken.IsCancellationRequested)
    {
        messages.Add("My message");
    }
}

Expected behavior

No exception is thrown.

Actual behavior

41:675
Unhandled exception. System.ArgumentException: Value does not fall within the expected range.
   at System.Collections.Immutable.ImmutableExtensions.ToArray[T](IEnumerable`1 sequence, Int32 count)
   at System.Collections.Immutable.ImmutableArray.CreateRange[T](IEnumerable`1 items)
   at Program.<Main>$(String[] args) in C:\Workspaces\AutoResetEventTests\AutoResetEventTests\Program.cs:line 24
   at Program.<Main>(String[] args)

Regression?

No - also an issue on NET 6.0/7.0 as well.

Known Workarounds

No response

Configuration

Version: NET 8.0

Other information

This test failure from a few years appears to be the exact same case, but with ToArray(), https://github.com/dotnet/runtime/issues/24465.

[dotnet-policy-service[bot]]

Tagging subscribers to this area: @dotnet/area-system-collections See info in area-owners.md if you want to be subscribed.

[stephentoub]

Duplicate of https://github.com/dotnet/runtime/issues/14502. Anything that uses ICollection.Count/CopyTo to copy the data out of a concurrent collection is not safe if the collection might change size concurrently.

[JakeYallop]

Ah - so I'm guess the fix in this case, if I don't want to lock on writes to the concurrent bag, is to use GetEnumerator directly (which may be what some of the other LINQ methods are doing and why they happen to work in this case).

Closing as its not a bug.

[colejohnson66]

I would argue that it's still a bug. If .ToArray().ToImmutableArray() works, why should .ToImmutableArray() fail?

[JakeYallop]

It is documented behaviour:

https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentbag-1?view=net-8.0

All public and protected members of ConcurrentBag are thread-safe and may be used concurrently from multiple threads. However, members accessed through one of the interfaces the ConcurrentBag implements, including extension methods, are not guaranteed to be thread safe and may need to be synchronized by the caller.

Just because some methods happen to work now, does not mean that they will continue to work as LINQ evolves. LINQ doesn't want to special case concurrent collections (as they are not used very often), nor does it want to use GetEnumerator if it can use CopyTo/Count and gain much better performance.

I'm not sure if its possible to explicitly implement CopyTo or Count inside the concurrent collections in order to make this behaviour "work" though.

[colejohnson66]

Ah I was not aware of the documentation warning. However, it still rubs me the wrong way. Sure, .Select(…).ToArray() wouldn’t be thread safe as it’s LINQ. However, a plain .ToArray() call, despite being LINQ based, doesn’t feel like a LINQ method to me. It’s not finalizing a pipeline, but more of a “collect” operation.

Regardless, you’re correct. It’s “working as designed”; I just don’t like it :)