Free Radical Free Radical - 1 year ago 74
C# Question

Locking on a collection, then creating a new instance of the collection. Dangerous?

I'm currently working on a multi-threaded interface for a few hardware devices, and I was storing my polling packet in a List collection. Since this interface needs to be very thread safe, I've taken to locking the collection every time I want to make changes to it. I've found through running diagnostics that creating a new instance of the collection is much faster than clearing it and then adding a new range of values for the poll. I was wanting to know if doing this though would cause issues with the locking, as I have limited experience with multi-threading applications and even less experience locking on a collection rather than a SyncRoot object.

Here's an example snippet for comparison:

lock (Poll)
Poll = new List<byte>(new byte[] { START, 0x00, STATUS_REQUEST, 0x00, 0x00});


lock (Poll)
Poll.AddRange(new byte[] { START, ZERO, STATUS_REQUEST, ZERO, ZERO });

Edit: Wanted to note that the Poll object is not publicly exposed. That's a no-no.

Answer Source

Since you've likely already read that you should not be locking on any public properties/this skipping that part of the problem.

lock(Poll) { Poll = new ... } is just wrong - don't even try; lock (Poll) { Poll.Clear();...{ is against "don't lock on public objects" guidance, but at least will achieve desired effect.

lock statement uses current value of variable as gate, if value changes later it may not properly block another thread from re-entering the same piece of code under lock.

lock(Poll) { Poll = new ... } will lock on Poll object and whoever have the same object cached locally can continue locking and being prevented from re-entering this code. Code that just lock(Poll) {...} will unlikely be able to get the same value of Poll variable when it is being assigned and probably enter the same block of code.

Correct code:

object lockPoll = new object(); // stays the same over `Poll` property lifetime

   // code that accesses or changes Poll 
   // both Poll = new ... or Poll.Clear() are fine
Recommended from our users: Dynamic Network Monitoring from WhatsUp Gold from IPSwitch. Free Download