flq/MemBus

ArgumentException when adding large number of subscriptions

Closed this issue · 5 comments

The following code will more often than not result in an ArgumentException being raised with the message "An item with the same key has already been added.":

var bus = BusSetup.StartWith<AsyncConfiguration>().Construct();

for (uint i = 0; i < 100000; i++)
{
    bus.Subscribe((int x) => Console.WriteLine(x));
}

Relevant part of the exception stack trace is

   at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at MemBus.CompositeSubscription.JustAdd(ISubscription subscription)
   at MemBus.CompositeSubscription.Add(ISubscription subscription)
   at MemBus.CompositeResolver.Add(ISubscription subscription)
   at MemBus.Subscriber.Subscribe[M](Action`1 subscription, ISubscriptionShaper customization)
   at MemBus.Subscriber.Subscribe[M](Action`1 subscription)
   at MemBus.MessageObservable`1.Subscribe(IObserver`1 observer)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)

My guess is that two DisposableSubscription instances have the same hashcode, resulting in a collision when attempting to add it the subscription storage:

_subscriptions.Add(disposableSub.GetHashCode(), disposableSub);

Changing the subscriptions storage to HashSet would be a possible fix as the default IEqualityComparer will check for equality (in this case reference equality) when getting multiple elements with the same hash code.

I'm getting the same exception with much lower number of subscriptions: ~20

flq commented

Hey, thanks for the PR, wondering why this was a Dictionary right now. Pre-Hashset-time?

How could you verify that this sorts you out?

I wanted to push a new version with netstandard as target, but I need to migrate the project.json stuff I have also.

I've added a unit test that @sidhoda suggested.
I cannot get the Test project to compile on my machine due to project.json references issue. I hope it will work on yours.
I've updated my project and haven't noticed the exception anymore.

flq commented

Sorry that your PR is sitting there, but I tried to upgrade to the new csproj stuff from .NET Core and looks like I failed, so I am doing another version with the project.json stuff and then hopefully I have the stuff up and running again.

flq commented

This has been fixed with commit e28770a.