From: Michal 'vorner' Vaner Date: Mon, 1 Oct 2012 12:24:51 +0000 (+0200) Subject: [2202] Minor improvements X-Git-Tag: trac2402_base~79^2~5^2~8 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=da7375d619b7ba63b77c8430f8d67799d8f26026;p=thirdparty%2Fkea.git [2202] Minor improvements * Documentation * Exception texts * Exception safety for a rare situation --- diff --git a/src/lib/util/threads/lock.h b/src/lib/util/threads/lock.h index dd714742ec..3a28f1a53a 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/thread.cc b/src/lib/util/threads/thread.cc index bbf7f513e2..87bce4c6a6 100644 --- a/src/lib/util/threads/thread.cc +++ b/src/lib/util/threads/thread.cc @@ -22,9 +22,12 @@ #include +#include + 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& 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_raw); + Impl* impl = static_cast(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& main) : impl_(NULL) { auto_ptr 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& 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 ex; - if (impl_->exception_) { - ex.reset(new UncaughtException(__FILE__, __LINE__, - impl_->exception_text_.c_str())); + scoped_ptr 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 4d14859130..9c6d20a4b6 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& main);