Merge lp:~michihenning/unity-scopes-api/reaper-hang into lp:unity-scopes-api

[Michi Henning]

Got rid of chatter from helgrind. (Needs helgrind 3.9.0 or later--3.8.1 generates bogus errors.)
Added public destroy() method so it is possible to shut down the reaper explicitly.
Fixed race condition on setting the self_ weak_ptr.

Got rid of chatter from helgrind. (Needs helgrind 3.9.0 or later--3.8.1 generates bogus errors.)
Added public destroy() method so it is possible to shut down the reaper explicitly.
Fixed race condition on setting the self_ weak_ptr.

[Michi Henning (michihenning)]

OK, I think I found it. The problem was that add() could run before self_ was set, so the back pointer form a reap item to the reaper was stale. Jussi, thanks again for the stack traces! Without those, I wouldn't have spotted this.

Can you give this a spin please and see whether it works for you now?

While I was at it, I got rid of the chatter from helgrind and added a public destroy() method as a convenience function.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:94
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/127/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-amd64-ci/127
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/127
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-armhf-ci/127/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-trusty-i386-ci/127

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/127/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

Let's approve this because it's good even though it will not fix the bug entirely.

review: Approve

=== modified file 'include/scopes/internal/Reaper.h'
--- include/scopes/internal/Reaper.h	2013-11-21 21:44:00 +0000
+++ include/scopes/internal/Reaper.h	2013-12-17 00:55:11 +0000
@@ -137,6 +137,10 @@
     // the total number of items).
     static SPtr create(int reap_interval, int expiry_interval, DestroyPolicy p = NoCallbackOnDestroy);
 
+    // Destroys the reaper and returns once any remaining items have been reaped (depending on the
+    // destroy policy). The destructor implicitly calls destroy().
+    void destroy();
+
     // Adds a new item to the reaper. The reaper calls cb once the item has not been refreshed for at
     // least expiry_interval seconds. O(1) performance.
     // The callback passed to add() must not block for any length of time. We have only one reaper
@@ -152,8 +156,9 @@
 private:
     Reaper(int reap_interval, int expiry_interval, DestroyPolicy p);
     void set_self() noexcept;
+    void start();
 
-    void run();                             // Start function for reaper thread
+    void reap_func();                       // Start function for reaper thread
 
     void remove_zombies(reaper_private::Reaplist const&) noexcept;   // Invokes callbacks for expired entries
 

=== modified file 'src/internal/Reaper.cpp'
--- src/internal/Reaper.cpp	2013-11-25 16:22:12 +0000
+++ src/internal/Reaper.cpp	2013-12-17 00:55:11 +0000
@@ -71,7 +71,7 @@
     auto const reaper = wp_reaper.lock();
     if (reaper)
     {
-        lock_guard<mutex> lock(reaper->mutex_);
+        lock_guard<mutex> reaper_lock(reaper->mutex_);
         assert(it_ != reaper->list_.end());
         reaper_private::Item item(*it_);
         item.timestamp = chrono::steady_clock::now();
@@ -101,7 +101,7 @@
     auto const reaper = wp_reaper.lock();
     if (reaper)
     {
-        lock_guard<mutex> lock(reaper->mutex_);
+        lock_guard<mutex> reaper_lock(reaper->mutex_);
         assert(it_ != reaper->list_.end());
         reaper->list_.erase(it_);
     }
@@ -129,18 +129,11 @@
         s << "Reaper: reap_interval (" << reap_interval << ") must be <= expiry_interval (" << expiry_interval << ").";
         throw unity::LogicException(s.str());
     }
-    reap_thread_ = thread(&Reaper::run, this);
 }
 
 Reaper::~Reaper() noexcept
 {
-    // Let the reaper thread know that it needs to stop doing things
-    {
-        lock_guard<mutex> lock(mutex_);
-        finish_ = true;
-    }
-    do_work_.notify_one();
-    reap_thread_.join();
+    destroy();
 }
 
 // Instantiate a new reaper. We call set_self() after instantiation so the reaper
@@ -152,6 +145,7 @@
 {
     SPtr reaper(new Reaper(reap_interval, expiry_interval, p));
     reaper->set_self();
+    reaper->start();
     return reaper;
 }
 
@@ -160,6 +154,26 @@
     self_ = shared_from_this();
 }
 
+void Reaper::start()
+{
+    reap_thread_ = thread(&Reaper::reap_func, this);
+}
+
+void Reaper::destroy()
+{
+    // Let the reaper thread know that it needs to stop doing things
+    {
+        lock_guard<mutex> lock(mutex_);
+        if (finish_)
+        {
+            return;
+        }
+        finish_ = true;
+        do_work_.notify_one();
+    }
+    reap_thread_.join();
+}
+
 // Add a new entry to the reaper. If the entry is not refreshed within the expiry interval,
 // the reaper removes the item from the list and calls cb to let the caller know about the expiry.
 
@@ -170,17 +184,19 @@
         throw unity::InvalidArgumentException("Reaper: invalid null callback passed to add().");
     }
 
+    lock_guard<mutex> lock(mutex_);
+
+    if (finish_)
+    {
+        throw unity::LogicException("Reaper: cannot add item to destroyed reaper.");
+    }
+
     // 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
+    Item item(cb);
+    list_.push_front(item); // LRU order
+    ri = list_.begin();
+    if (list_.size() == 1)
     {
         do_work_.notify_one();  // Wake up reaper thread
     }
@@ -201,7 +217,7 @@
 
 // Reaper thread
 
-void Reaper::run()
+void Reaper::reap_func()
 {
     unique_lock<mutex> lock(mutex_);
     for (;;)
@@ -225,8 +241,7 @@
             auto const reap_interval = chrono::duration_cast<chrono::milliseconds>(reap_interval_);
             auto const sleep_interval = max(expiry_interval_ - oldest_item_age, reap_interval);
             auto const wakeup_time = now + sleep_interval;
-            while (!finish_ && do_work_.wait_until(lock, wakeup_time) != cv_status::timeout)
-                ;
+            do_work_.wait_until(lock, wakeup_time, [this]{ return finish_; });
         }
 
         if (finish_ && policy_ == NoCallbackOnDestroy)