BIND 10 trac2202, updated. c4e44c1a57326915e7291e982e2271b6574f73fc [2202] Test improvements
BIND 10 source code commits
bind10-changes at lists.isc.org
Mon Oct 1 13:13:28 UTC 2012
The branch, trac2202 has been updated
via c4e44c1a57326915e7291e982e2271b6574f73fc (commit)
via da7375d619b7ba63b77c8430f8d67799d8f26026 (commit)
from 6e3d1a58e4ec1857fc0264e8070ce67f569b7e42 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit c4e44c1a57326915e7291e982e2271b6574f73fc
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Mon Oct 1 15:10:50 2012 +0200
[2202] Test improvements
* Reimplementing destroyLocked as death test
* Comments
* Includes to use < > instead of " "
* Don't use c99-isms.
commit da7375d619b7ba63b77c8430f8d67799d8f26026
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Mon Oct 1 14:24:51 2012 +0200
[2202] Minor improvements
* Documentation
* Exception texts
* Exception safety for a rare situation
-----------------------------------------------------------------------
Summary of changes:
src/lib/util/threads/lock.h | 6 +--
src/lib/util/threads/tests/lock_unittest.cc | 54 +++++++++++++++----------
src/lib/util/threads/tests/thread_unittest.cc | 4 +-
src/lib/util/threads/thread.cc | 45 +++++++++++++++------
src/lib/util/threads/thread.h | 10 ++++-
5 files changed, 76 insertions(+), 43 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/util/threads/lock.h b/src/lib/util/threads/lock.h
index dd71474..3a28f1a 100644
--- a/src/lib/util/threads/lock.h
+++ b/src/lib/util/threads/lock.h
@@ -67,10 +67,6 @@ public:
/// Destroyes 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.
@@ -107,7 +103,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() {
diff --git a/src/lib/util/threads/tests/lock_unittest.cc b/src/lib/util/threads/tests/lock_unittest.cc
index 7ffc654..748580c 100644
--- a/src/lib/util/threads/tests/lock_unittest.cc
+++ b/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,8 +12,8 @@
// 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>
@@ -50,21 +50,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 +70,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,29 +81,38 @@ 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,
+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;
}
}
@@ -112,15 +120,17 @@ performStrangeOperation(long long unsigned* array, int direction,
TEST(MutexTest, swarm) {
// 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];
diff --git a/src/lib/util/threads/tests/thread_unittest.cc b/src/lib/util/threads/tests/thread_unittest.cc
index f94c359..97cace2 100644
--- a/src/lib/util/threads/tests/thread_unittest.cc
+++ b/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>
diff --git a/src/lib/util/threads/thread.cc b/src/lib/util/threads/thread.cc
index bbf7f51..87bce4c 100644
--- a/src/lib/util/threads/thread.cc
+++ b/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
@@ -126,17 +138,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) {
diff --git a/src/lib/util/threads/thread.h b/src/lib/util/threads/thread.h
index 4d14859..9c6d20a 100644
--- a/src/lib/util/threads/thread.h
+++ b/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);
More information about the bind10-changes
mailing list