Fixing #334 by making AttributesToAvoidReplicating thread safe#335
Fixing #334 by making AttributesToAvoidReplicating thread safe#335jonorossi merged 3 commits intocastleproject:masterfrom BrunoJuchli:master
Conversation
…collection instead of modifying it.
…ncurrently replacing the collection.
|
while (!(originalAttributes = attributes).Contains(attribute))
|
stakx
left a comment
There was a problem hiding this comment.
Just a few comments regarding your use of lock, in case you end up using this kind of lock. (Castle has its own lock type based on ReaderWriterLockSlim I believe.)
|
|
||
| public static class AttributesToAvoidReplicating | ||
| { | ||
| private static readonly object lockObject = new object(); |
There was a problem hiding this comment.
Is this needed? I think you could just lock on attributes directly.
There was a problem hiding this comment.
As the list is not accessible externally of the class this is of course a viable alternative.
The only reason not to do so is to prevent future issues where collections could be made available externally and an external consumer could lock it. Let's say you'd introduce
public static IReadOnlyList<Type> Types => attributes;
I'm fine either way. I won't go around locking other "people's" objects, so I most likely won't be dealing with that issue, should it ever arise ;-)
| IList<Type> originalAttributes; | ||
| while (!(originalAttributes = attributes).Contains(attribute)) | ||
| // note: this class is made thread-safe by replacing the backing list rather than adding to it | ||
| lock (lockObject) |
There was a problem hiding this comment.
I think you also need to put locks around places where attributes is accessed for reading; that is, in Contains and in ShouldAvoid. (Not sure about the former, but almost certain in case of the latter.)
There was a problem hiding this comment.
IMO not necessary. Exchanging object references is atomic. So Contains and ShouldAvoid will either get the old reference or the new reference.
Bug #334 is fixed by just replacing the list instead of extending it. However, the lock is necessary to safeguard against concurrent calls to Add. Even though they are not very likely, of course.
There was a problem hiding this comment.
Oops, I mis-read the diff (or rather, I didn't read all of it). You're absolutely right, of course.
|
Thanks @BrunoJuchli, merged. |
This PR fixes #334 by making
AttributesToAvoidReplicatingthread safe by replacing the backing collection instead of updating it (...while consumers are enumerating it...).It employs
Interlocked.CompareExchangeto replace the collection if it's still the same as the original. If replacing fails (because another thread has already exchanged the collection) it retries until it succeeds.Alternative would have been to replace the backing collection but to
locktheAddmethod so concurrentAdd-ing would be sequentialised.