Merge pull request #838 from Expensify/tyler-fix-scope-issue

Fix deleting locked mutex

sqlitecluster/SQLiteSequentialNotifier.cpp

@@ -28,12 +28,30 @@ void SQLiteSequentialNotifier::notifyThrough(uint64_t value) {
     if (value > _value) {
         _value = value;
     }
-    while (!_valueToPendingThreadMap.empty() && _valueToPendingThreadMap.begin()->first <= value) {
-        lock_guard<mutex> lock(_valueToPendingThreadMap.begin()->second->waitingThreadMutex);
-        _valueToPendingThreadMap.begin()->second->result = RESULT::COMPLETED;
-        _valueToPendingThreadMap.begin()->second->waitingThreadConditionVariable.notify_all();
-        _valueToPendingThreadMap.erase(_valueToPendingThreadMap.begin());
+    auto lastToDelete = _valueToPendingThreadMap.begin();
+    for (auto it = _valueToPendingThreadMap.begin(); it != _valueToPendingThreadMap.end(); it++) {
+        if (it->first > value)  {
+            // If we've passed our value, there's nothing else to erase, so we can stop.
+            break;
+        }
+
+        // Note that we'll delete this item from the map.
+        lastToDelete++;
+
+        // Make the changes to the state object - mark it complete and notify anyone waiting.
+        lock_guard<mutex> lock(it->second->waitingThreadMutex);
+        it->second->result = RESULT::COMPLETED;
+        it->second->waitingThreadConditionVariable.notify_all();
     }
+
+    // Now we've finished with all of our updates and notifications and can remove everything from our map.
+    // Note that erasing an empty range (i.e., from() begin to begin()) is tested to be a no-op. The documentation I've
+    // fond for multimap is unclear on this, though the docuemtnation for `std::list` specifies:
+    // "The iterator first does not need to be dereferenceable if first==last: erasing an empty range is a no-op."
+    //
+    // I think it's reasonable to assume this is the intention for multimap as well, and in my testing, that was the
+    // case.
+    _valueToPendingThreadMap.erase(_valueToPendingThreadMap.begin(), lastToDelete);
 }
 
 void SQLiteSequentialNotifier::cancel() {