Merge lp:~michihenning/unity-scopes-api/signal-before-unlock into lp:unity-scopes-api

[Michi Henning]

Changed code to signal condvar while still holding the lock. No correctness issue here. The change is mainly to stop warnings from helgrind, which doesn't like signals outside a lock.

Changed code to signal condvar while still holding the lock. No correctness issue here. The change is mainly to stop warnings from helgrind, which doesn't like signals outside a lock.

[Marcus Tomlinson (marcustomlinson)]

I believe we should always attempt to have mutexes lock and unlock at their most efficient points in code (Its one of those optimisations you can foresee while coding). I see RAII mutexes as great safety nets when making a mistake, or when a scope is exited unexpectedly. My point is, perhaps we should not remove those lock.unlock() lines completely, but instead move them a line below the notify. This not only gives the thread a few more clock ticks now, but later allows for code to be added to these methods without causing the mutex to be held any longer than it needs to.

review: Needs Information

[Michi Henning (michihenning)]

Hmmm... Pretty much all the places where there is a notify or notify_all, that is immediately followed by a close of hte scope, a break, or a return, which all do the unlock.

Which unlock() calls would you restore and put after the notify?

[Marcus Tomlinson (marcustomlinson)]

Ok sure, apart from the one removed from ScopeLoader.cpp, all mutexes are unlocked implicitly almost immediately. This is fine provided that the methods stay this way. If one of these methods were extended further, or a complex temporary were added and required clean up, we could cause the mutex to be held longer unintentionally in the future. Its your call really, just putting it out there :) The fix is correct and working.

[Marcus Tomlinson (marcustomlinson)]

review: Approve

[Michi Henning (michihenning)]

Well, if I add code later that depends on holding a lock, I need to examine the lock scopes anyway, just as I do if I add code later that shouldn't hold a lock. It's just something we need to think about when changing threaded code, no matter what convention we use to unlock.

The unlock() in ScopeLoader.cpp could stay and would release the mutex a little earlier. But then we end up with noise from helgrind again when the notify happens further down if we took the code path where the unlock() call was.

I think, on balance, it's probably better to leave it as is. At least until we find that it's broken :-)

=== modified file 'demo/client.cpp'
--- demo/client.cpp	2013-09-23 06:13:06 +0000
+++ demo/client.cpp	2013-10-25 04:17:24 +0000
@@ -41,10 +41,8 @@
     virtual void finished() override
     {
         cout << "query complete" << endl;
-        {
-            unique_lock<decltype(mutex_)> lock(mutex_);
-            query_complete_ = true;
-        }
+        unique_lock<decltype(mutex_)> lock(mutex_);
+        query_complete_ = true;
         condvar_.notify_one();
     }
 
 
=== modified file 'demo/scope-C.cpp'
--- demo/scope-C.cpp	2013-08-28 06:26:46 +0000
+++ demo/scope-C.cpp	2013-10-25 04:17:24 +0000
@@ -44,10 +44,8 @@
 public:
     void put(MyQuery const* query, string const& query_string, ReplyProxy const& reply_proxy)
     {
-        {
-            std::lock_guard<std::mutex> lock(mutex_);
-            queries_.push_back(QueryData { query, query_string, reply_proxy });
-        }
+        std::lock_guard<std::mutex> lock(mutex_);
+        queries_.push_back(QueryData { query, query_string, reply_proxy });
         condvar_.notify_one();
     }
 
@@ -57,7 +55,6 @@
         condvar_.wait(lock, [this] { return !queries_.empty() || done_; });
         if (done_)
         {
-            lock.unlock();
             condvar_.notify_all();
         }
         else
@@ -88,11 +85,9 @@
 
     void finish()
     {
-        {
-            std::unique_lock<std::mutex> lock(mutex_);
-            queries_.clear();
-            done_ = true;
-        }
+        std::unique_lock<std::mutex> lock(mutex_);
+        queries_.clear();
+        done_ = true;
         condvar_.notify_all();
     }
 
 
=== modified file 'demo/scope-D.cpp'
--- demo/scope-D.cpp	2013-08-28 06:26:46 +0000
+++ demo/scope-D.cpp	2013-10-25 04:17:24 +0000
@@ -47,10 +47,8 @@
 public:
     void put(MyQuery const* query, string const& query_string, ReplyProxy const& reply_proxy)
     {
-        {
-            std::lock_guard<std::mutex> lock(mutex_);
-            queries_.push_back(QueryData { query, query_string, reply_proxy });
-        }
+        std::lock_guard<std::mutex> lock(mutex_);
+        queries_.push_back(QueryData { query, query_string, reply_proxy });
         condvar_.notify_one();
     }
 
@@ -60,7 +58,6 @@
         condvar_.wait(lock, [this] { return !queries_.empty() || done_; });
         if (done_)
         {
-            lock.unlock();
             condvar_.notify_all();
         }
         else
@@ -91,11 +88,9 @@
 
     void finish()
     {
-        {
-            std::unique_lock<std::mutex> lock(mutex_);
-            queries_.clear();
-            done_ = true;
-        }
+        std::unique_lock<std::mutex> lock(mutex_);
+        queries_.clear();
+        done_ = true;
         condvar_.notify_all();
     }
 
 
=== modified file 'src/unity/api/scopes/internal/Reaper.cpp'
--- src/unity/api/scopes/internal/Reaper.cpp	2013-08-27 06:28:14 +0000
+++ src/unity/api/scopes/internal/Reaper.cpp	2013-10-25 04:17:24 +0000
@@ -126,8 +126,8 @@
     {
         lock_guard<mutex> lock(mutex_);
         finish_ = true;
+        do_work_.notify_one();
     }
-    do_work_.notify_one();
     reap_thread_.join();
 }
 
@@ -161,17 +161,15 @@
 
     // Put new Item at the head of the list.
     reaper_private::Reaplist::iterator ri;
-    size_t list_size;
     {
         lock_guard<mutex> lock(mutex_);
         Item item(cb);
         list_.push_front(item); // LRU order
         ri = list_.begin();
-        list_size = list_.size();
-    }
-    if (list_size == 1) // List just became non-empty
-    {
-        do_work_.notify_one();  // Wake up reaper thread
+        if (list_.size() == 1) // List just became non-empty
+        {
+            do_work_.notify_one();  // Wake up reaper thread
+        }
     }
 
     // Make a new ReapItem.
 
=== modified file 'src/unity/api/scopes/internal/ReplyObject.cpp'
--- src/unity/api/scopes/internal/ReplyObject.cpp	2013-08-27 06:28:14 +0000
+++ src/unity/api/scopes/internal/ReplyObject.cpp	2013-10-25 04:17:24 +0000
@@ -117,7 +117,6 @@
     lock.lock();
     if (--num_push_ == 0)
     {
-        lock.unlock();
         idle_.notify_one();
     }
 }
 
=== modified file 'src/unity/api/scopes/internal/ScopeLoader.cpp'
--- src/unity/api/scopes/internal/ScopeLoader.cpp	2013-08-20 05:25:28 +0000
+++ src/unity/api/scopes/internal/ScopeLoader.cpp	2013-10-25 04:17:24 +0000
@@ -145,8 +145,8 @@
         // FALLTHROUGH
         case ScopeState::Finished:
         {
+            cmd_changed_.notify_all();
             lock.unlock();
-            cmd_changed_.notify_all();
             if (run_thread_.joinable())            // If unload is called more than once, don't join a second time.
             {
                 run_thread_.join();
@@ -183,11 +183,9 @@
     {
         case ScopeState::Stopped:
         {
-            {
-                // cppcheck-suppress unreadVariable
-                lock_guard<mutex> lock(cmd_mutex_);
-                cmd_ = ScopeCmd::Start;
-            }
+            // cppcheck-suppress unreadVariable
+            lock_guard<mutex> lock(cmd_mutex_);
+            cmd_ = ScopeCmd::Start;
             cmd_changed_.notify_all();
             break;
         }
@@ -225,12 +223,11 @@
     {
         case ScopeState::Started:
         {
-            {
-                // cppcheck-suppress unreadVariable
-                lock_guard<mutex> lock(cmd_mutex_);
-                cmd_ = ScopeCmd::Stop;
-            }
+            // cppcheck-suppress unreadVariable
+            lock_guard<mutex> lock(cmd_mutex_);
+            cmd_ = ScopeCmd::Stop;
             cmd_changed_.notify_all();
+            break;
         }
         case ScopeState::Stopped:
         case ScopeState::Finished:
@@ -286,26 +283,22 @@
         unity::util::ResourcePtr<ScopeBase*, decltype(destroy_func)> scope_base(create_func(), destroy_func);
         if (scope_base.get() == nullptr)
         {
-            {
-                // cppcheck-suppress unreadVariable
-                unique_lock<mutex> lock(state_mutex_);
-                scope_state_ = ScopeState::Failed;
-                exception_ = make_exception_ptr(
-                                 unity::ResourceException("Scope " + scope_name_ +
-                                                          " returned nullptr from " + UNITY_API_SCOPE_CREATE_SYMSTR));
-            }
+            // cppcheck-suppress unreadVariable
+            unique_lock<mutex> lock(state_mutex_);
+            scope_state_ = ScopeState::Failed;
+            exception_ = make_exception_ptr(
+                             unity::ResourceException("Scope " + scope_name_ +
+                                                      " returned nullptr from " + UNITY_API_SCOPE_CREATE_SYMSTR));
             state_changed_.notify_all();
             return;
         }
 
         // Notify the parent thread that we are ready to go.
         {
-            {
-                // cppcheck-suppress unreadVariable
-                unique_lock<mutex> lock(state_mutex_);
-                scope_state_ = ScopeState::Stopped;
-                scope_base_ = scope_base.get();
-            }
+            // cppcheck-suppress unreadVariable
+            unique_lock<mutex> lock(state_mutex_);
+            scope_state_ = ScopeState::Stopped;
+            scope_base_ = scope_base.get();
             state_changed_.notify_all();
         }
 
@@ -397,7 +390,6 @@
                                          unity::ResourceException("scope " + scope_name_ + ": unknown exception in stop()"));
                         throw;
                     }
-                    lock.unlock();
                     if (run_thread_.joinable())
                     {
                         run_thread_.join();
@@ -436,11 +428,9 @@
 
 void ScopeLoader::handle_thread_exception()
 {
-    {
-        // cppcheck-suppress unreadVariable
-        lock_guard<mutex> lock(state_mutex_);
-        scope_state_ = ScopeState::Failed;
-    }
+    // cppcheck-suppress unreadVariable
+    lock_guard<mutex> lock(state_mutex_);
+    scope_state_ = ScopeState::Failed;
     state_changed_.notify_all();
 }
 
@@ -465,24 +455,20 @@
             exception_ = make_exception_ptr(
                              unity::ResourceException("Scope " + scope_name_ + ": exception in run()"));
             scope_state_ = ScopeState::Failed;
+            state_changed_.notify_all();
         }
-        state_changed_.notify_all();
 
-        {
-            // cppcheck-suppress unreadVariable
-            lock_guard<mutex> lock(cmd_mutex_);
-            cmd_ = ScopeCmd::Stop;
-        }
+        // cppcheck-suppress unreadVariable
+        lock_guard<mutex> lock(cmd_mutex_);
+        cmd_ = ScopeCmd::Stop;
         cmd_changed_.notify_all();
     }
 }
 
 void ScopeLoader::notify_app_thread_started()
 {
-    {
-        unique_lock<mutex> lock(run_thread_mutex_);
-        run_thread_state_ = AppState::Started;
-    }
+    unique_lock<mutex> lock(run_thread_mutex_);
+    run_thread_state_ = AppState::Started;
     run_thread_changed_.notify_all();
 }
 
 
=== modified file 'src/unity/api/scopes/internal/zmq_middleware/ObjectAdapter.cpp'
--- src/unity/api/scopes/internal/zmq_middleware/ObjectAdapter.cpp	2013-10-16 15:04:14 +0000
+++ src/unity/api/scopes/internal/zmq_middleware/ObjectAdapter.cpp	2013-10-25 04:17:24 +0000
@@ -152,7 +152,6 @@
         {
             run_workers();
             state_ = Active;
-            lock.unlock();
             state_changed_.notify_all();
             break;
         }
@@ -200,7 +199,6 @@
             ServantMap().swap(servants_);   // Not need for a try block. The ServantBase destructor is noexcept.
             lock.lock();
             state_ = Inactive;
-            lock.unlock();
             state_changed_.notify_all();
             break;
         }
 
=== modified file 'src/unity/api/scopes/internal/zmq_middleware/ZmqMiddleware.cpp'
--- src/unity/api/scopes/internal/zmq_middleware/ZmqMiddleware.cpp	2013-10-16 15:04:14 +0000
+++ src/unity/api/scopes/internal/zmq_middleware/ZmqMiddleware.cpp	2013-10-25 04:17:24 +0000
@@ -109,7 +109,6 @@
             // TODO: get pool size from config
             invokers_.reset(new ThreadPool(1));
             state_ = Started;
-            lock.unlock();
             state_changed_.notify_all();
             break;
         }
@@ -149,7 +148,6 @@
                 pair.second->wait_for_shutdown();
             }
             state_ = Stopped;
-            lock.unlock();
             state_changed_.notify_all();
             break;
         }