Missing lock in HashMap set() and other functions

General JUCE discussion
1

Whilst I normally use HashMap without a CriticalSection template argument I noticed there may be some potentially missing locks when using a CriticalSection, particularly in the set() function? If so it seems to have been introduced some time ago here:

Before this change the CriticalSection was locked for the whole duration of the set() function but now the assignment of the value to the hash map entry is outside the critical section. Two calls on different threads could compete to set a different value for the same key here. And possibly worse (although I haven’t checked in detail) the whole table could be remapped in between getReference() and the assignment when another thread interleaves and is setting a key that causes the table to be remapped. I haven’t checked the algorithm to see if this invalidates the reference returned by getReference() but if not then I guess the lock could be inserted between getReference() and the assignement instead.

I have a diff here:

@@ -160,6 +160,7 @@
     /** Returns the current number of items in the map. */
     inline int size() const noexcept
     {
+        const ScopedLockType sl (getLock());
         return totalNumItems;
     }

@@ -229,7 +230,11 @@
         If there's already an item with the given key, this will replace its value. Otherwise, a new item
         will be added to the map.
     */
-    void set (KeyTypeParameter newKey, ValueTypeParameter newValue)        { getReference (newKey) = newValue; }
+    void set (KeyTypeParameter newKey, ValueTypeParameter newValue)
+    {
+        const ScopedLockType sl (getLock());
+        getReference (newKey) = newValue;
+    }

     /** Removes an item with the given key. */
     void remove (KeyTypeParameter keyToRemove)
@@ -331,6 +336,7 @@
     */
     inline int getNumSlots() const noexcept
     {
+        const ScopedLockType sl (getLock());
         return hashSlots.size();
     }
1 Like