Index out of range when using GetInstance(type)

[kutensky]

Fixes #186. Use ConcurrentBag instead of List, because Disposables collection might be called from multiple threads and it should be thread-safe

[CodingGorilla]

I don't think this change is required, and I think it just adds unnecessary overhead. You only need to worry about locking and concurrency during reads and removals, writes are totally fine from multiple threads. And this collection is only ever written to, until the container is disposed, at which point nothing else should be using the container anyways.

If people are using this list in user code, then that's on them to implement and lock appropriately.

[kutensky]

@CodingGorilla We faced this issue on production code after switching from StructureMap to Lamar. It happens for us when we use Lamar as DependencyResolver for controllers in MVC 5. When there are too many simultaneous calls of "GetInstance" from Lamar of disposable objects, it puts these objects to the Disposables list and sometimes throws an exception on "Add" method call (as shown in the original ticket). It's reproducible on the unit test I committed. You just need to replace "ConcurrentBad" with "List" to make it reproduce again.

[remcoros]

@CodingGorilla operations on List are not thread safe. See https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.list-1?redirectedfrom=MSDN&view=netframework-4.8#c9721fa0-1cd9-4a21-818c-98d164c9fc14 (Thread Safety)

Why? because if multiple threads are reading/mutating the same list, and it so happens that the internal array of elements needs a resize, it will be copied to a new array. That resize operation is not thread safe

[CodingGorilla]

@remcoros I understand your point, but what we're talking about here is a list which should be almost exclusively written to, and not read from. The only read time should be when the container is being disposed, and I can't imagine a real scenario where a separate thread is still adding things to the disposables list while the container is being disposed. And if that is the case you're going to get a leak because the items that are added after the disposal will never get disposed. In this case, I almost think the exception would be preferable.

[kutensky]

If you navigate to original issue #186 you can find the bug this fix is for. The scenario is the following: Many separate threads call "Add" method of List. In some moment of time List understand that internal array is too small and need to be extended. It extends an array, but meanwhile, other threads wait because of Lock. When "lock" is released they all add new items to the list without check for length and suddenly we got the "Out of range" exception. You can reproduce it by running unit test I added to the pull request (you need to replace ConcurrentBag to List to see it reproducing). This is not just a theoretical scenario that might happen. This is a real scenario that happened on our production code under pressure. And someone else (author of the ticket) also had this exception. So I'm not the only one.

[CodingGorilla]

@kutensky I added your test (with no other changes) to the master branch of the code and have not been able to get the test to fail in > 20 runs. Maybe this is a framework specific issue? I'm using Lamar with .NET Core 2.x & 3.x and have never seen the problem. I guess I'll leave this up to @jeremydmiller to decide.

[jeremydmiller]

This will be in Lamar 4.1 soon