[2202] Merge branch 'trac2202' with the remote version.

fixing conflicts, apparently due to a missing push:
	src/lib/util/threads/tests/lock_unittest.cc

src/bin/auth/auth_srv.cc

@@ -629,13 +629,13 @@ AuthSrvImpl::processNormalQuery(const IOMessage& io_message, Message& message,
         local_edns->setUDPSize(AuthSrvImpl::DEFAULT_LOCAL_UDPSIZE);
         message.setEDNS(local_edns);
     }
+    // Lock the client lists and keep them under the lock until the processing
+    // and rendering is done (this is the same mutex as from
+    // AuthSrv::getClientListMutex()).
+    isc::util::thread::Mutex::Locker locker(mutex_);
 
     try {
         const ConstQuestionPtr question = *message.beginQuestion();
-        // Lock the client lists and keep them under the lock until
-        // the processing is done (this is the same mutex as from
-        // AuthSrv::getClientListMutex()).
-        isc::util::thread::Mutex::Locker locker(mutex_);
         const boost::shared_ptr<datasrc::ClientList>
             list(getClientList(question->getClass()));
         if (list) {
@@ -664,6 +664,8 @@ AuthSrvImpl::processNormalQuery(const IOMessage& io_message, Message& message,
     LOG_DEBUG(auth_logger, DBG_AUTH_MESSAGES, AUTH_SEND_NORMAL_RESPONSE)
               .arg(renderer_.getLength()).arg(message);
     return (true);
+    // The message can contain some data from the locked resource. But outside
+    // this method, we touch only the RCode of it, so it should be safe.
 }
 
 bool

src/bin/auth/tests/auth_srv_unittest.cc

@@ -1802,7 +1802,9 @@ TEST_F(AuthSrvTest, clientList) {
 // We just test the mutex can be locked (exactly once).
 TEST_F(AuthSrvTest, mutex) {
     isc::util::thread::Mutex::Locker l1(server.getClientListMutex());
-    // TODO: Once we have non-debug build, this one will not work.
+    // TODO: Once we have non-debug build, this one will not work, since
+    // we currently use the fact that we can't lock twice from the same
+    // thread. In the non-debug mode, this would deadlock.
     // Skip then.
     EXPECT_THROW({
         isc::util::thread::Mutex::Locker l2(server.getClientListMutex());

src/lib/util/threads/lock.cc

@@ -56,7 +56,7 @@ struct Deinitializer {
 
 }
 
-Mutex::Mutex(bool recursive) :
+Mutex::Mutex() :
     impl_(NULL)
 {
     pthread_mutexattr_t attributes;
@@ -72,11 +72,7 @@ Mutex::Mutex(bool recursive) :
     Deinitializer deinitializer(attributes);
     // TODO: Distinguish if debug mode is enabled in compilation.
     // If so, it should be PTHREAD_MUTEX_NORMAL or NULL
-    int type = PTHREAD_MUTEX_ERRORCHECK;
-    if (recursive) {
-        type = PTHREAD_MUTEX_RECURSIVE;
-    }
-    result = pthread_mutexattr_settype(&attributes, type);
+    result = pthread_mutexattr_settype(&attributes, PTHREAD_MUTEX_ERRORCHECK);
     if (result != 0) {
         isc_throw(isc::InvalidOperation, strerror(result));
     }

src/lib/util/threads/lock.h

@@ -44,33 +44,24 @@ class Mutex : public boost::noncopyable {
 public:
     /// \brief Constructor.
     ///
-    /// Creates a mutex. Depending on the parameter, it is either recursive
-    /// (the same thread may lock it multiple times, others wait; it must be
-    /// unlocked as many times to become really unlocked) or normal (can be
-    /// locked just once, if the same threads tries to lock it again, Bad
-    /// Things Happen).
+    /// Creates a mutex. It is a non-recursive mutex (can be locked just once,
+    /// if the same threads tries to lock it again, Bad Things Happen).
     ///
     /// Depending on compilation parameters and OS, the mutex may or may not
     /// do some error and sanity checking. However, such checking is meant
     /// only to aid development, not rely on it as a feature.
     ///
-    /// \param recursive If the thread should be recursive (lockable multiple
-    ///     times from the same thread) or not.
     /// \throw std::bad_alloc In case allocation of something (memory, the
     ///     OS mutex) fails.
     /// \throw isc::InvalidOperation Other unspecified errors around the mutex.
     ///     This should be rare.
-    Mutex(bool recursive = false);
+    Mutex();
 
     /// \brief Destructor.
     ///
     /// Destroys the mutex. It is not allowed to destroy a mutex which is
     /// currently locked. This means a Locker created with this Mutex must
     /// never live longer than the Mutex itself.
-    ///
-    /// \throw isc::InvalidOperation when the OS reports an error. This should
-    ///     generally happen only when the Mutex was used in a wrong way,
-    ///     meaning programmer error.
     ~Mutex();
 
     /// \brief This holds a lock on a Mutex.
@@ -90,8 +81,7 @@ public:
         ///
         /// \throw isc::InvalidOperation when OS reports error. This usually
         ///     means an attempt to use the mutex in a wrong way (locking
-        ///     a non-recursive mutex a second time from the same thread,
-        ///     for example).
+        ///     a mutex second time from the same thread, for example).
         Locker(Mutex& mutex) :
             mutex_(NULL)
         {
@@ -107,7 +97,7 @@ public:
         ///
         /// Unlocks the mutex.
         ///
-        /// \throw isc::InvalidOperation when OS repotrs error. This usually
+        /// \throw isc::InvalidOperation when OS reports error. This usually
         ///     means an attempt to use the mutex in a wrong way (unlocking
         ///     a mutex belonging to a differen thread).
         ~Locker() {

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

@@ -1,4 +1,4 @@
-// Copyright (C) 2010  Internet Systems Consortium, Inc. ("ISC")
+// 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
@@ -12,29 +12,19 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include "../lock.h"
-#include "../thread.h"
+#include <util/threads/lock.h>
+#include <util/threads/thread.h>
 
 #include <gtest/gtest.h>
 
 #include <boost/bind.hpp>
+#include <unistd.h>
+#include <signal.h>
 
 using namespace isc::util::thread;
 
 namespace {
 
-// Test a recursive mutex can be locked multiple times
-TEST(MutexTest, recursiveLockMultiple) {
-    Mutex mutex(true);
-    EXPECT_FALSE(mutex.locked()); // Debug-only build
-    Mutex::Locker l1(mutex);
-    EXPECT_TRUE(mutex.locked()); // Debug-only build
-    Mutex::Locker l2(mutex);
-    Mutex::Locker l3(mutex);
-    Mutex::Locker l4(mutex);
-    Mutex::Locker l5(mutex);
-}
-
 // If we try to lock the debug mutex multiple times, it should throw.
 TEST(MutexTest, lockMultiple) {
     // TODO: Once we support non-debug mutexes, disable the test if we compile
@@ -50,21 +40,17 @@ TEST(MutexTest, lockMultiple) {
 }
 
 // Destroying a locked mutex is a bad idea as well
-//
-// FIXME: The test is disabled, since it screws something up in the VM (other
-// tests fail then with rather cryptic messages, memory dumps and stuff).
-// Any idea how to make the test work and reasonably safe?
-TEST(MutexTest, DISABLED_destroyLocked) {
-    // TODO: This probably won't work for non-debug mutexes. Disable on
-    // non-debug compilation.
-    Mutex* mutex = new Mutex;
-    new Mutex::Locker(*mutex);
-    EXPECT_THROW(delete mutex, isc::InvalidOperation);
-    // Note: This leaks the locker. But this is a test for development aid
-    // exception. The exception won't happen in normal build anyway and seeing
-    // it means there's a bug. And we can't delete the locker now, since it
-    // would access uninitialized memory.
+#ifdef EXPECT_DEATH
+TEST(MutexTest, destroyLocked) {
+    EXPECT_DEATH({
+        Mutex* mutex = new Mutex;
+        new Mutex::Locker(*mutex);
+        delete mutex;
+        // This'll leak the locker, but inside the slave process, it should
+        // not be an issue.
+    }, "");
 }
+#endif
 
 // This test tries if a mutex really locks. We could try that with a deadlock,
 // but that's not practical (the test would not end).
@@ -74,6 +60,9 @@ TEST(MutexTest, DISABLED_destroyLocked) {
 // threads are started and the operation must be complicated enough so the
 // compiler won't turn it into some kind of single atomic instruction.
 //
+// FIXME: Any idea for a simpler but non-atomic operation that keeps an
+// invariant?
+//
 // So, we'll have an array of numbers. Each thread will try to repeatedly
 // find a number large at least as half of the average, take the number,
 // distribute the value across the rest of the positions of the array and
@@ -82,49 +71,77 @@ TEST(MutexTest, DISABLED_destroyLocked) {
 // one will add something to the position another one have chosen and
 // the other one will then zero it, not taking the new value into account.
 // That'd lower the total value of the array.
+//
+// We run the threads in opposite directions (so we have no chance of them
+// keeping the same distance to each other and not meeting). Also, the indexing
+// is performed in a circular manner like with a ring buffer.
 const unsigned long long length = 1000;
 const unsigned long long iterations = 10000;
 const unsigned long long value = 2000;
 void
-performStrangeOperation(long long unsigned* array, int direction, Mutex* mutex)
+performStrangeOperation(std::vector<long long unsigned>& array, int direction,
+                        Mutex* mutex)
 {
     unsigned long long position = 0;
     for (size_t i = 0; i < iterations; ++i) {
         Mutex::Locker lock(*mutex);
+        // Find a place with large enough value
         while (array[position % length] < value) {
             position += direction;
         }
-        unsigned long long value = array[position % length];
+        // Take the value
+        unsigned long long found_value = array[position % length];
+        // And distribute it to places following the found one, by
+        // adding 1 to each.
         unsigned long long p2 = position;
-        while (value > 0) {
+        while (found_value > 0) {
             p2 += direction;
             if (p2 % length == position % length) {
                 continue;
             }
             ++array[p2 % length];
-            --value;
+            --found_value;
         }
+        // Zero the distributed value
         array[position % length] = 0;
     }
 }
 
+void
+no_handler(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;
+    if (sigaction(SIGALRM, &ignored, &original)) {
+        FAIL() << "Couldn't set alarm";
+    }
+    alarm(10);
     // This type has a low chance of being atomic itself, further raising
     // the chance of problems appearing.
-    long long unsigned array[length];
+    std::vector<long long unsigned> array(length);
     for (size_t i = 0; i < length; ++i) {
         array[i] = value;
     }
     Mutex mutex;
+    // Run two parallel threads, each in one direction
     Thread t1(boost::bind(&performStrangeOperation, array, 1, &mutex));
     Thread t2(boost::bind(&performStrangeOperation, array, -1, &mutex));
     t1.wait();
     t2.wait();
+    // Check the sum didn't change.
     long long unsigned sum = 0;
     for (size_t i = 0; i < length; ++i) {
         sum += array[i];
     }
     EXPECT_EQ(length * value, sum) << "Threads are badly synchronized";
+    // Cancel the alarm and return the original handler
+    alarm(0);
+    if (sigaction(SIGALRM, &original, NULL)) {
+        FAIL() << "Couldn't restore alarm";
+    }
 }
 
 }

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

@@ -1,4 +1,4 @@
-// Copyright (C) 2010  Internet Systems Consortium, Inc. ("ISC")
+// 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
@@ -12,7 +12,7 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include "../thread.h"
+#include <util/threads/thread.h>
 
 #include <boost/bind.hpp>
 
@@ -36,16 +36,14 @@ const size_t iterations = 200;
 const size_t detached_iterations = 25;
 
 void
-doSomething(int* x) {
-    delete[] x;
-}
+doSomething(int*) { }
 
 // We just test that we can forget about the thread and nothing
 // bad will happen on our side.
 TEST(ThreadTest, detached) {
+    int x;
     for (size_t i = 0; i < detached_iterations; ++i) {
-        int* x = new int[10];
-        Thread thread(boost::bind(&doSomething, x));
+        Thread thread(boost::bind(&doSomething, &x));
     }
 }
 
@@ -72,18 +70,28 @@ throwSomething() {
     throw 42; // Throw something really unusual, to see everything is caught.
 }
 
+void
+throwException() {
+    throw std::exception();
+}
+
 // Exception in the thread we forget about should not do anything to us
 TEST(ThreadTest, detachedException) {
     for (size_t i = 0; i < detached_iterations; ++i) {
         Thread thread(throwSomething);
     }
+    for (size_t i = 0; i < detached_iterations; ++i) {
+        Thread thread(throwException);
+    }
 }
 
 // An uncaught exception in the thread should propagate through wait
 TEST(ThreadTest, exception) {
     for (size_t i = 0; i < iterations; ++i) {
         Thread thread(throwSomething);
+        Thread thread2(throwException);
         ASSERT_THROW(thread.wait(), Thread::UncaughtException);
+        ASSERT_THROW(thread2.wait(), Thread::UncaughtException);
     }
 }
 

src/lib/util/threads/thread.cc

@@ -22,9 +22,12 @@
 
 #include <pthread.h>
 
+#include <boost/scoped_ptr.hpp>
+
 using std::string;
 using std::exception;
 using std::auto_ptr;
+using boost::scoped_ptr;
 
 namespace isc {
 namespace util {
@@ -41,6 +44,8 @@ namespace thread {
 class Thread::Impl {
 public:
     Impl(const boost::function<void ()>& main) :
+        // Two things to happen before destruction - thread needs to terminate
+        // and the creating thread needs to release it.
         waiting_(2),
         main_(main),
         exception_(false)
@@ -50,7 +55,7 @@ public:
     static void done(Impl* impl) {
         bool should_delete(false);
         { // We need to make sure the mutex is unlocked before it is deleted
-            Mutex::Locker locker(impl->mutex);
+            Mutex::Locker locker(impl->mutex_);
             if (--impl->waiting_ == 0) {
                 should_delete = true;
             }
@@ -61,16 +66,15 @@ public:
     }
     // Run the thread. The type of parameter is because the pthread API.
     static void* run(void* impl_raw) {
-        Impl* impl = reinterpret_cast<Impl*>(impl_raw);
+        Impl* impl = static_cast<Impl*>(impl_raw);
         try {
             impl->main_();
         } catch (const exception& e) {
-            Mutex::Locker locker(impl->mutex);
             impl->exception_ = true;
             impl->exception_text_ = e.what();
         } catch (...) {
-            Mutex::Locker locker(impl->mutex);
             impl->exception_ = true;
+            impl->exception_text_ = "Uknown exception";
         }
         done(impl);
         return (NULL);
@@ -84,16 +88,24 @@ public:
     // Was there an exception?
     bool exception_;
     string exception_text_;
-    Mutex mutex;
+    // The mutex protects the waiting_ member, which ensures there are
+    // no race conditions and collisions when terminating. The other members
+    // should be safe, because:
+    // * tid_ is read only.
+    // * exception_ and exception_text_ is accessed outside of the thread
+    //   only after join, by that time the thread must have terminated.
+    // * main_ is used in a read-only way here. If there are any shared
+    //   resources used inside, it is up to the main_ itself to take care.
+    Mutex mutex_;
     // Which thread are we talking about anyway?
-    pthread_t tid;
+    pthread_t tid_;
 };
 
 Thread::Thread(const boost::function<void ()>& main) :
     impl_(NULL)
 {
     auto_ptr<Impl> impl(new Impl(main));
-    const int result = pthread_create(&impl->tid, NULL, &Impl::run,
+    const int result = pthread_create(&impl->tid_, NULL, &Impl::run,
                                       impl.get());
     // Any error here?
     switch (result) {
@@ -110,7 +122,7 @@ Thread::Thread(const boost::function<void ()>& main) :
 Thread::~Thread() {
     if (impl_ != NULL) {
         // In case we didn't call wait yet
-        const int result = pthread_detach(impl_->tid);
+        const int result = pthread_detach(impl_->tid_);
         Impl::done(impl_);
         impl_ = NULL;
         // If the detach ever fails, something is screwed rather badly.
@@ -125,17 +137,24 @@ Thread::wait() {
                   "Wait called and no thread to wait for");
     }
 
-    const int result = pthread_join(impl_->tid, NULL);
+    const int result = pthread_join(impl_->tid_, NULL);
     if (result != 0) {
         isc_throw(isc::InvalidOperation, strerror(result));
     }
 
     // Was there an exception in the thread?
-    auto_ptr<UncaughtException> ex;
-    if (impl_->exception_) {
-        ex.reset(new UncaughtException(__FILE__, __LINE__,
-                                       impl_->exception_text_.c_str()));
+    scoped_ptr<UncaughtException> ex;
+    try { // Something in here could in theory throw.
+        if (impl_->exception_) {
+            ex.reset(new UncaughtException(__FILE__, __LINE__,
+                                           impl_->exception_text_.c_str()));
+        }
+    } catch (...) {
+        Impl::done(impl_);
+        impl_ = NULL;
+        throw;
     }
+
     Impl::done(impl_);
     impl_ = NULL;
     if (ex.get() != NULL) {

src/lib/util/threads/thread.h

@@ -22,6 +22,11 @@
 
 namespace isc {
 namespace util {
+/// \brief Wrappers for thread related functionality
+///
+/// We provide our own wrappers, currently around pthreads. We tried using
+/// the boost thread support, but it gave us some trouble, so we implemented
+/// in-house ones.
 namespace thread {
 
 /// \brief A separate thread.
@@ -58,6 +63,9 @@ public:
     /// may just want to use boost::bind or alike to store them within the
     /// body functor.
     ///
+    /// \note The main functor will be copied internally. You need to consider
+    ///     this when returning the result.
+    ///
     /// The body should terminate by exiting the function. If it throws, it
     /// is considered an error. You should generally catch any exceptions form
     /// within there and handle them somehow.
@@ -65,7 +73,7 @@ public:
     /// \param main The code to run inside the thread.
     ///
     /// \throw std::bad_alloc if allocation of the new thread or other
-    /// resources fails.
+    ///     resources fails.
     /// \throw isc::InvalidOperation for other errors (should not happen).
     Thread(const boost::function<void()>& main);