[2205] Merge branch 'trac2332' into trac2205

src/lib/util/threads/lock.cc

@@ -110,19 +110,35 @@ Mutex::~Mutex() {
 }
 
 void
+Mutex::postLockAction() {
+    if (impl_->locked_count != 0) {
+        isc_throw(isc::InvalidOperation, "Lock attempt for locked mutex");
+    }
+    ++impl_->locked_count;
+}
+
+void
 Mutex::lock() {
     assert(impl_ != NULL);
     const int result = pthread_mutex_lock(&impl_->mutex);
     if (result != 0) {
         isc_throw(isc::InvalidOperation, std::strerror(result));
     }
-    ++impl_->locked_count; // Only in debug mode
+    postLockAction();           // Only in debug mode
+}
+
+void
+Mutex::preUnlockAction() {
+    if (impl_->locked_count == 0) {
+        isc_throw(isc::InvalidOperation, "Unlock attempt for unlocked mutex");
+    }
+    --impl_->locked_count;
 }
 
 void
 Mutex::unlock() {
     assert(impl_ != NULL);
-    --impl_->locked_count; // Only in debug mode
+    preUnlockAction();          // Only in debug mode
     const int result = pthread_mutex_unlock(&impl_->mutex);
     assert(result == 0); // This should never be possible
 }
@@ -133,6 +149,59 @@ Mutex::locked() const {
     return (impl_->locked_count != 0);
 }
 
+class CondVar::Impl {
+public:
+    Impl() {
+        const int result = pthread_cond_init(&cond_, NULL);
+        if (result != 0) {
+            isc_throw(isc::Unexpected, "pthread_cond_init failed: "
+                      << std::strerror(result));
+        }
+    }
+    ~Impl() {
+        const int result = pthread_cond_destroy(&cond_);
+
+        // This can happen if we try to destroy cond_ while some other thread
+        // is waiting on it.  assert() may be too strong for such a case,
+        // but we cannot safely destroy cond_ anyway.  In order to avoid
+        // throwing from a destructor we simply let the process die.
+        assert(result == 0);
+    }
+
+    // For convenience allow the main class to access this directly.
+    pthread_cond_t cond_;
+};
+
+CondVar::CondVar() : impl_(new Impl)
+{}
+
+CondVar::~CondVar() {
+    delete impl_;
+}
+
+void
+CondVar::wait(Mutex& mutex) {
+    mutex.preUnlockAction();    // Only in debug mode
+    const int result = pthread_cond_wait(&impl_->cond_, &mutex.impl_->mutex);
+    mutex.postLockAction();     // Only in debug mode
+
+    // pthread_cond_wait should normally succeed unless mutex is completely
+    // broken.
+    if (result != 0) {
+        isc_throw(isc::BadValue, "pthread_cond_wait failed unexpectedly: " <<
+                  std::strerror(result));
+    }
+}
+
+void
+CondVar::signal() {
+    const int result = pthread_cond_signal(&impl_->cond_);
+
+    // pthread_cond_signal() can only fail when if cond_ is invalid.  It
+    //should be impossible as long as this is a valid CondVar object.
+    assert(result == 0);
+}
+
 }
 }
 }

src/lib/util/threads/lock.h

@@ -22,6 +22,7 @@
 namespace isc {
 namespace util {
 namespace thread {
+class CondVar;
 
 /// \brief Mutex with very simple interface
 ///
@@ -41,7 +42,7 @@ namespace thread {
 ///
 /// The current interface is somewhat minimalistic. If we ever need more, we
 /// can add it later.
-class Mutex : public boost::noncopyable {
+class Mutex : boost::noncopyable {
 public:
     /// \brief Constructor.
     ///
@@ -74,7 +75,7 @@ public:
     /// collecting" mechanism (auto_ptr, for example), it ensures exception
     /// safety with regards to the mutex - it'll get released on the exit
     /// of function no matter by what means.
-    class Locker : public boost::noncopyable {
+    class Locker : boost::noncopyable {
     public:
         /// \brief Constructor.
         ///
@@ -84,26 +85,19 @@ public:
         ///     means an attempt to use the mutex in a wrong way (locking
         ///     a mutex second time from the same thread, for example).
         Locker(Mutex& mutex) :
-            mutex_(NULL)
+            mutex_(&mutex)
         {
-            // Set the mutex_ after we acquire the lock. This is because of
-            // exception safety. If lock() throws, it didn't work, so we must
-            // not unlock when we are destroyed. In such case, mutex_ is
-            // NULL and checked in the destructor.
             mutex.lock();
-            mutex_ = &mutex;
         }
 
         /// \brief Destructor.
         ///
         /// Unlocks the mutex.
         ~Locker() {
-            if (mutex_ != NULL) {
-                mutex_->unlock();
-            }
+            mutex_->unlock();
         }
     private:
-        Mutex* mutex_;
+        Mutex* const mutex_;
     };
     /// \brief If the mutex is currently locked
     ///
@@ -114,15 +108,106 @@ public:
     /// \todo Disable in non-debug build
     bool locked() const;
 private:
+    friend class CondVar;
+
+    // Commonly called after acquiring the lock, checking and updating
+    // internal state for debug.
+    void postLockAction();
+
+    // Commonly called before releasing the lock, checking and updating
+    // internal state for debug.
+    void preUnlockAction();
+
     class Impl;
     Impl* impl_;
     void lock();
     void unlock();
 };
 
+/// \brief Encapsulation for a condition variable.
+///
+/// This class provides a simple encapsulation of condition variable for
+/// inter-thread synchronization.  It has similar but simplified interface as
+/// that for \c pthread_bond_ variants.
+///
+/// It uses the \c Mutex class object for the mutex used with the condition
+/// variable.  Since for normal applications the internal \c Mutex::Locker
+/// class is the only available interface to acquire a lock, sample code
+/// for waiting on a condition variable would look like this:
+/// \code
+/// CondVar cond;
+/// Mutex mutex;
+/// {
+///     Mutex::Locker locker(mutex);
+///     while (some_condition) {
+///         cond.wait(mutex);
+///     }
+///     // do something under the protection of locker
+/// }   // lock is released here
+/// \endcode
+/// Note that \c mutex passed to the \c wait() method must be the same one
+/// used to construct the \c locker.
+///
+/// \note This class is defined as a friend class of \c Mutex and directly
+/// refers to and modifies private internals of the \c Mutex class.  It breaks
+/// the assumption that the lock is only acquired or released via the
+/// \c Locker class and breaks other integrity assumption on \c Mutex,
+/// thereby making it more fragile, but we couldn't find other way to
+/// implement a safe and still simple realization of condition variables.
+/// So, this is a kind of compromise.  If this class is needed to be
+/// extended, first consider a way to use public interfaces of \c Mutex;
+/// do not easily rely on the fact that this class is a friend of it.
+class CondVar : boost::noncopyable {
+public:
+    /// \brief Constructor.
+    ///
+    /// \throw std::bad_alloc memory allocation failure
+    /// \throw isc::Unexpected other unexpected shortage of system resource
+    CondVar();
 
-}
-}
-}
+    /// \brief Destructor.
+    ///
+    /// An object of this class must not be destructed while some thread
+    /// is waiting on it.  If this condition isn't met the destructor will
+    /// terminate the program.
+    ~CondVar();
+
+    /// \brief Wait on the condition variable.
+    ///
+    /// This method works like \c pthread_cond_wait().  For mutex it takes
+    /// an \c Mutex class object.  Unlike \c pthread_cond_wait(), however,
+    /// this method requires a lock for the mutex has been acquired.
+    /// If this condition isn't met, it can throw an exception (in the
+    /// debug mode build) or result in undefined behavior.
+    ///
+    /// The lock will be automatically released within this method, and
+    /// will be re-acquired on the exit of this method.
+    ///
+    /// \throw isc::InvalidOperation mutex isn't locked
+    /// \throw isc::BadValue mutex is not a valid \c Mutex object
+    ///
+    /// \param mutex A \c Mutex object to be released on wait().
+    void wait(Mutex& mutex);
+
+    /// \brief Unblock a thread waiting for the condition variable.
+    ///
+    /// This method waits one of other threads (if any) waiting on this object
+    /// via the \c wait() call.
+    ///
+    /// This method never throws; if some unexpected low level error happens
+    /// it terminates the program.
+    void signal();
+private:
+    class Impl;
+    Impl* impl_;
+};
+
+} // namespace thread
+} // namespace util
+} // namespace isc
 
 #endif
+
+// Local Variables:
+// mode: c++
+// End:

src/lib/util/threads/tests/Makefile.am

@@ -23,6 +23,7 @@ TESTS += run_unittests
 run_unittests_SOURCES  = run_unittests.cc
 run_unittests_SOURCES += thread_unittest.cc
 run_unittests_SOURCES += lock_unittest.cc
+run_unittests_SOURCES += condvar_unittest.cc
 
 run_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
 run_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS) $(PTHREAD_LDFLAGS)

src/lib/util/threads/tests/condvar_unittest.cc

@@ -0,0 +1,169 @@
+// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <exceptions/exceptions.h>
+
+#include <util/threads/lock.h>
+#include <util/threads/thread.h>
+
+#include <gtest/gtest.h>
+
+#include <boost/bind.hpp>
+#include <boost/scoped_ptr.hpp>
+
+#include <cstring>
+
+#include <unistd.h>
+#include <signal.h>
+
+using namespace isc::util::thread;
+
+namespace {
+// Used as a signal handler below.
+volatile bool do_exit;          // use for emergency escape
+void
+alarmHandler(int) {
+    do_exit = true;
+}
+
+class CondVarTest : public ::testing::Test {
+protected:
+    // We use a signal in case some of the thread synchronization tests
+    // unexpectedly cause a deadlock.
+    void SetUp() {
+        do_exit = false;
+
+        std::memset(&handler_, 0, sizeof(handler_));
+        handler_.sa_handler = alarmHandler;
+        if (sigaction(SIGALRM, &handler_, &original_) != 0) {
+            FAIL() << "Couldn't set alarm";
+        }
+        alarm(10);                  // 10sec duration: arbitrary choice
+    }
+    void TearDown() {
+        // Cancel the alarm and return the original handler
+        alarm(0);
+        if (sigaction(SIGALRM, &original_, NULL)) {
+            FAIL() << "Couldn't restore alarm";
+        }
+    }
+
+    CondVar condvar_;
+    Mutex mutex_;
+private:
+    struct sigaction handler_, original_;
+};
+
+TEST(CondVarTest0, create) {
+    // Just construct and destruct it.  Nothing unusual should happen.
+    EXPECT_NO_THROW(CondVar condvar);
+}
+
+// Running on a separate thread, just updating the argument and waking up
+// the other thread via the condition variable passed.
+void
+ringSignal(CondVar* condvar, Mutex* mutex, int* arg) {
+    assert(*arg == 0);
+    Mutex::Locker locker(*mutex);
+    ++*arg;
+    condvar->signal();
+}
+
+// A simple wait-signal operation on a condition variable.
+TEST_F(CondVarTest, waitAndSignal) {
+    Mutex::Locker locker(mutex_);
+    int shared_var = 0; // let the other thread increment this
+    Thread t(boost::bind(&ringSignal, &condvar_, &mutex_, &shared_var));
+    condvar_.wait(mutex_);
+    t.wait();
+    EXPECT_EQ(1, shared_var);
+}
+
+// Thread's main code for the next test
+void
+signalAndWait(CondVar* condvar1, CondVar* condvar2, Mutex* mutex, int* arg) {
+    Mutex::Locker locker(*mutex);
+    ++*arg;
+    condvar2->signal();  // let the main thread know this one is ready
+    condvar1->wait(*mutex);
+    ++*arg;
+}
+
+// Similar to the previous test, but having two threads wait for a condvar.
+TEST_F(CondVarTest, multiWaits) {
+    boost::scoped_ptr<Mutex::Locker> locker(new Mutex::Locker(mutex_));
+    CondVar condvar2; // separate cond var for initial synchronization
+    int shared_var = 0; // let the other thread increment this
+    Thread t1(boost::bind(&signalAndWait, &condvar_, &condvar2, &mutex_,
+                          &shared_var));
+    Thread t2(boost::bind(&signalAndWait, &condvar_, &condvar2, &mutex_,
+                          &shared_var));
+
+    // Wait until both threads are waiting on condvar_.
+    while (shared_var < 2 && !do_exit) {
+        condvar2.wait(mutex_);
+    }
+    // Check we exited from the loop successfully.
+    ASSERT_FALSE(do_exit);
+    ASSERT_EQ(2, shared_var);
+
+    // release the lock, wake up both threads, wait for them to die, and
+    // confirm they successfully woke up.
+    locker.reset();
+    condvar_.signal();
+    condvar_.signal();
+    t1.wait();
+    t2.wait();
+    EXPECT_EQ(4, shared_var);
+}
+
+void
+waiter(CondVar* condver, Mutex* mutex) {
+    Mutex::Locker locker(*mutex);
+    condver->wait(*mutex);
+}
+
+// Similar to the previous version of the same function, but just do
+// condvar operations.  It will never wake up.
+void
+signalAndWait(CondVar* condvar, Mutex* mutex) {
+    Mutex::Locker locker(*mutex);
+    condvar->signal();
+    condvar->wait(*mutex);
+}
+
+#ifdef EXPECT_DEATH
+TEST_F(CondVarTest, bad) {
+    // We'll destroy a CondVar object while the thread is still waiting
+    // on it.  This will trigger an assertion failure.
+    EXPECT_DEATH({
+            CondVar cond;
+            Mutex::Locker locker(mutex_);
+            Thread t(boost::bind(&signalAndWait, &cond, &mutex_));
+            cond.wait(mutex_);
+        }, "");
+}
+#endif
+
+TEST_F(CondVarTest, badWait) {
+    // In our implementation, wait() requires acquiring the lock beforehand.
+    EXPECT_THROW(condvar_.wait(mutex_), isc::InvalidOperation);
+}
+
+TEST_F(CondVarTest, emptySignal) {
+    // It's okay to call signal when no one waits.
+    EXPECT_NO_THROW(condvar_.signal());
+}
+
+}

src/lib/util/threads/tests/lock_unittest.cc

@@ -80,13 +80,13 @@ performIncrement(volatile double* canary, volatile bool* ready_me,
 }
 
 void
-no_handler(int) {}
+noHandler(int) {}
 
 TEST(MutexTest, swarm) {
     // Create a timeout in case something got stuck here
     struct sigaction ignored, original;
-    memset(&ignored, 0, sizeof ignored);
-    ignored.sa_handler = no_handler;
+    memset(&ignored, 0, sizeof(ignored));
+    ignored.sa_handler = noHandler;
     if (sigaction(SIGALRM, &ignored, &original)) {
         FAIL() << "Couldn't set alarm";
     }