search

datalust / seq-api

HTTP API client for Seq
https://datalust.co/seq
Apache License 2.0
78 stars 21 forks source link

Allow Reentrancy in SeqConnection LoadResourceGroupAsync() #10

Closed bbrandt closed 8 years ago

bbrandt commented 8 years ago

I started looking at this because I was receiving an "item with the same key has already been added" error from the Dictionary in SeqConnection.

I had to drop support for Windows Phone Silverlight to gain access to System.Collections.Concurrent. This changed from Profile259 to Profile111.

Updated to the latest Microsoft.Net.Http, Microsoft.Bcl.Build, and Microsoft.Bcl while I was there.

nblumhardt commented 8 years ago

Thanks Ben.

Currently, the whole of Seq.Api is single-threaded; just to be clarify, is your scenario multi-threaded or single?

I can't see any problems with moving to a thread-safe version, but there may be a few more things to check elsewhere in the codebase if so (happy to help find these).

bbrandt commented 8 years ago

Sorry, didn't get a chance to answer this yesterday. Yes. I realized the single-threaded-ness of the library early on when I tried to do a few different async calls against the library concurrently and await Task.WhenAll().

After correcting these places to be serialized calls, we still ran into an issue where our WPF view's loaded event gets fired twice in quick succession, I suspect because of animations. In the loaded event we are calling await seqConnection.Signals.ListAsync() (through a layer or two of abstraction). Both the calls were getting past the if (_resourceGroups.TryGetValue(name, out loaded)) in LoadResourceGroupAsync and both trying to insert the same key into resourceGroups. I was multi-threaded without intending to be.

We will eventually fix our issue with the loaded event, but I thought it a good opportunity to fix the issue I was running into in Seq.Api so I don't bump into that particular issue again.

Thanks for merging.