Valkryst/VRadio

addReceiver, removeReceiver, and removeReceivers are not thread safe

Closed this issue · 4 comments

While you are using a ConcurrentDictionary, which is great, these two methods are not thread safe.

addReceiver

Consider. N threads can call add on the same HashSet concurrently. HashSet is not threadsafe.

removeReceiver

N threads can call remove on the same Hashset concurrently (with different arguments). Again, not threadsafe.

removeReceivers

Threads can concurrently be adding and clearing this hashset.

In order to remedy these, you're going to want to take advantage of the concurrent features that ConcurrentDictionary has to offer, such as compute or merge

A word of warning - the compute and merge functions can themselves by run concurrently, so you'll likely want to be returning entirely new copies of the hashsets, instead of simply calling .add/remove/etc on them.

Thanks for taking a look at the code. I had a sneaking suspicion that something wasn't right, but I couldn't put my finger on it until now.

I'm considering switching from HashSet to ConcurrentSkipListSet, what do you think?

ConcurrentSkipListSet will solve the concurrency problems :) But it also changes the container properties (O(log(n)) from O(n))

Sets in the java std library tend to be implemented as a Map<T, null>. This means you could also do something like this (set from map)

Thanks for the help. I've updated the Radio class to make use of newKeySet() over HashSet.