trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jpe...@apache.org
Subject [trafficserver] 01/02: TS-4952: Improve abort messages for mutex failures.
Date Tue, 09 May 2017 16:51:50 GMT
This is an automated email from the ASF dual-hosted git repository.

jpeach pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git

commit 472ec283ebd8366f6bd98f3154f524ccb8e12f7b
Author: James Peach <jpeach@apache.org>
AuthorDate: Mon Oct 10 19:57:31 2016 -0700

    TS-4952: Improve abort messages for mutex failures.
    
    Although the ink_mutex APIs claimed to return error values, they
    actually abort on failure. Improve the abort to format a message
    that includes the error, and don't bother to return error values
    any more since these can't fail.
    
    Since we now check for mutex manipulation errors, error checking
    mutexes (the default type on FreeBSD) will fail when we unlock a
    Thread mutex from a different thread than the one that created it.
    To solve this, we introduce TSThreadWait so that the creating thread
    can safely spawn a thread, wait for it and then destroy it.
---
 configure.ac                  |  1 +
 iocore/eventsystem/P_Thread.h |  4 ---
 iocore/eventsystem/Thread.cc  |  9 +++++-
 lib/records/I_RecMutex.h      |  8 ++---
 lib/records/RecMutex.cc       | 14 ++++-----
 lib/ts/ink_mutex.cc           | 20 +++++++++++--
 lib/ts/ink_mutex.h            | 63 +++++++++++++++++++-------------------
 lib/ts/ink_rwlock.cc          | 32 ++++++--------------
 lib/ts/ink_thread.h           |  1 +
 mgmt/api/EventControlMain.cc  |  8 +----
 proxy/InkAPITest.cc           |  5 ++++
 proxy/InkIOCoreAPI.cc         | 70 ++++++++++++++++++++++++++++++++++++++-----
 proxy/api/ts/ts.h             |  1 +
 13 files changed, 145 insertions(+), 91 deletions(-)

diff --git a/configure.ac b/configure.ac
index 1d22bdb..7401e1e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1091,6 +1091,7 @@ dnl Linux has pthread symbol stubss in both libc and libpthread, so
it's importa
 dnl specifically for pthread_yield() here. In addition, ASAN hijacks pthread_create() so
 dnl we can't use that anymore.
 AC_SEARCH_LIBS([pthread_yield], [pthread], [], [])
+AC_CHECK_FUNCS([pthread_mutexattr_settype])
 
 dnl XXX The following check incorrectly causes the build to succeed
 dnl on Darwin. We should be using AC_SEARCH_LIBS, but rest_init is
diff --git a/iocore/eventsystem/P_Thread.h b/iocore/eventsystem/P_Thread.h
index c7cce80..259f1ad 100644
--- a/iocore/eventsystem/P_Thread.h
+++ b/iocore/eventsystem/P_Thread.h
@@ -36,10 +36,6 @@
 ///////////////////////////////////////////////
 // Common Interface impl                     //
 ///////////////////////////////////////////////
-TS_INLINE
-Thread::~Thread()
-{
-}
 
 TS_INLINE void
 Thread::set_specific()
diff --git a/iocore/eventsystem/Thread.cc b/iocore/eventsystem/Thread.cc
index d0c3712..af04099 100644
--- a/iocore/eventsystem/Thread.cc
+++ b/iocore/eventsystem/Thread.cc
@@ -44,7 +44,14 @@ Thread::Thread()
 {
   mutex = new_ProxyMutex();
   MUTEX_TAKE_LOCK(mutex, (EThread *)this);
-  mutex->nthread_holding = THREAD_MUTEX_THREAD_HOLDING;
+  mutex->nthread_holding += THREAD_MUTEX_THREAD_HOLDING;
+}
+
+Thread::~Thread()
+{
+  ink_release_assert(mutex->thread_holding == (EThread *)this);
+  mutex->nthread_holding -= THREAD_MUTEX_THREAD_HOLDING;
+  MUTEX_UNTAKE_LOCK(mutex, (EThread *)this);
 }
 
 static void
diff --git a/lib/records/I_RecMutex.h b/lib/records/I_RecMutex.h
index d175a06..fcf37ac 100644
--- a/lib/records/I_RecMutex.h
+++ b/lib/records/I_RecMutex.h
@@ -38,9 +38,9 @@ struct RecMutex {
   ink_mutex the_mutex;
 };
 
-int rec_mutex_init(RecMutex *m, const char *name = nullptr);
-int rec_mutex_destroy(RecMutex *m);
-int rec_mutex_acquire(RecMutex *m);
-int rec_mutex_release(RecMutex *m);
+void rec_mutex_init(RecMutex *m, const char *name = nullptr);
+void rec_mutex_destroy(RecMutex *m);
+void rec_mutex_acquire(RecMutex *m);
+void rec_mutex_release(RecMutex *m);
 
 #endif
diff --git a/lib/records/RecMutex.cc b/lib/records/RecMutex.cc
index a03a8e6..f56fc42 100644
--- a/lib/records/RecMutex.cc
+++ b/lib/records/RecMutex.cc
@@ -24,23 +24,23 @@
 #include "ts/ink_config.h"
 #include "I_RecMutex.h"
 
-int
+void
 rec_mutex_init(RecMutex *m, const char *name)
 {
   m->nthread_holding = 0;
   m->thread_holding  = ink_thread_null();
-  return ink_mutex_init(&(m->the_mutex), name);
+  ink_mutex_init(&(m->the_mutex), name);
 }
 
-int
+void
 rec_mutex_destroy(RecMutex *m)
 {
   ink_assert(m->nthread_holding == 0);
   ink_assert(m->thread_holding == ink_thread_null());
-  return ink_mutex_destroy(&(m->the_mutex));
+  ink_mutex_destroy(&(m->the_mutex));
 }
 
-int
+void
 rec_mutex_acquire(RecMutex *m)
 {
   ink_thread this_thread = ink_thread_self();
@@ -51,10 +51,9 @@ rec_mutex_acquire(RecMutex *m)
   }
 
   m->nthread_holding++;
-  return 0;
 }
 
-int
+void
 rec_mutex_release(RecMutex *m)
 {
   if (m->nthread_holding != 0) {
@@ -64,5 +63,4 @@ rec_mutex_release(RecMutex *m)
       ink_mutex_release(&(m->the_mutex));
     }
   }
-  return 0;
 }
diff --git a/lib/ts/ink_mutex.cc b/lib/ts/ink_mutex.cc
index a088189..4d45f09 100644
--- a/lib/ts/ink_mutex.cc
+++ b/lib/ts/ink_mutex.cc
@@ -27,7 +27,21 @@
 #include <cstdio>
 #include "ts/ink_mutex.h"
 
-// Define the _g_mattr first to avoid static initialization order fiasco.
-x_pthread_mutexattr_t _g_mattr;
-
 ink_mutex __global_death = PTHREAD_MUTEX_INITIALIZER;
+
+x_pthread_mutexattr_t::x_pthread_mutexattr_t()
+{
+  pthread_mutexattr_init(&attr);
+#ifndef POSIX_THREAD_10031c
+  pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
+#endif
+
+#if DEBUG && HAVE_PTHREAD_MUTEXATTR_SETTYPE
+  pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_ERRORCHECK);
+#endif
+}
+
+x_pthread_mutexattr_t::~x_pthread_mutexattr_t()
+{
+  pthread_mutexattr_destroy(&attr);
+}
diff --git a/lib/ts/ink_mutex.h b/lib/ts/ink_mutex.h
index 9d6f78a..c25b7ae 100644
--- a/lib/ts/ink_mutex.h
+++ b/lib/ts/ink_mutex.h
@@ -35,6 +35,7 @@
 #include <stdio.h>
 
 #include "ts/ink_defs.h"
+#include "ts/ink_error.h"
 
 #if defined(POSIX_THREAD)
 #include <pthread.h>
@@ -48,61 +49,57 @@ class x_pthread_mutexattr_t
 {
 public:
   pthread_mutexattr_t attr;
+
   x_pthread_mutexattr_t();
-  ~x_pthread_mutexattr_t() {}
+  ~x_pthread_mutexattr_t();
 };
-inline x_pthread_mutexattr_t::x_pthread_mutexattr_t()
-{
-  pthread_mutexattr_init(&attr);
-#ifndef POSIX_THREAD_10031c
-  pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
-#endif
-}
 
-extern class x_pthread_mutexattr_t _g_mattr;
-
-static inline int
-ink_mutex_init(ink_mutex *m, const char *name)
+static inline void
+ink_mutex_init(ink_mutex *m, const char * /* name */)
 {
-  (void)name;
+  int error;
+  x_pthread_mutexattr_t attr;
 
-#if defined(solaris)
-  if (pthread_mutex_init(m, nullptr) != 0) {
-    abort();
-  }
-#else
-  if (pthread_mutex_init(m, &_g_mattr.attr) != 0) {
-    abort();
+  error = pthread_mutex_init(m, &attr.attr);
+  if (unlikely(error != 0)) {
+    ink_abort("pthread_mutex_init(%p) failed: %s (%d)", m, strerror(error), error);
   }
-#endif
-  return 0;
 }
 
-static inline int
+static inline void
 ink_mutex_destroy(ink_mutex *m)
 {
-  return pthread_mutex_destroy(m);
+  int error;
+
+  error = pthread_mutex_destroy(m);
+  if (unlikely(error != 0)) {
+    ink_abort("pthread_mutex_destroy(%p) failed: %s (%d)", m, strerror(error), error);
+  }
 }
 
-static inline int
+static inline void
 ink_mutex_acquire(ink_mutex *m)
 {
-  if (pthread_mutex_lock(m) != 0) {
-    abort();
+  int error;
+
+  error = pthread_mutex_lock(m);
+  if (unlikely(error != 0)) {
+    ink_abort("pthread_mutex_lock(%p) failed: %s (%d)", m, strerror(error), error);
   }
-  return 0;
 }
 
-static inline int
+static inline void
 ink_mutex_release(ink_mutex *m)
 {
-  if (pthread_mutex_unlock(m) != 0) {
-    abort();
+  int error;
+
+  error = pthread_mutex_unlock(m);
+  if (unlikely(error != 0)) {
+    ink_abort("pthread_mutex_unlock(%p) failed: %s (%d)", m, strerror(error), error);
   }
-  return 0;
 }
 
-static inline int
+static inline bool
 ink_mutex_try_acquire(ink_mutex *m)
 {
   return pthread_mutex_trylock(m) == 0;
diff --git a/lib/ts/ink_rwlock.cc b/lib/ts/ink_rwlock.cc
index f5943ac..a4a9af8 100644
--- a/lib/ts/ink_rwlock.cc
+++ b/lib/ts/ink_rwlock.cc
@@ -32,10 +32,7 @@
 int
 ink_rwlock_init(ink_rwlock *rw)
 {
-  int result;
-
-  if ((result = ink_mutex_init(&rw->rw_mutex, nullptr)) != 0)
-    goto Lerror;
+  ink_mutex_init(&rw->rw_mutex, nullptr);
 
   ink_cond_init(&rw->rw_condreaders);
   ink_cond_init(&rw->rw_condwriters);
@@ -46,9 +43,6 @@ ink_rwlock_init(ink_rwlock *rw)
   rw->rw_magic    = RW_MAGIC;
 
   return 0;
-
-Lerror:
-  return result; /* an errno value */
 }
 
 //-------------------------------------------------------------------------
@@ -78,13 +72,10 @@ ink_rwlock_destroy(ink_rwlock *rw)
 int
 ink_rwlock_rdlock(ink_rwlock *rw)
 {
-  int result;
-
   if (rw->rw_magic != RW_MAGIC)
     return EINVAL;
 
-  if ((result = ink_mutex_acquire(&rw->rw_mutex)) != 0)
-    return result;
+  ink_mutex_acquire(&rw->rw_mutex);
 
   /* give preference to waiting writers */
   while (rw->rw_refcount < 0 || rw->rw_nwaitwriters > 0) {
@@ -106,13 +97,10 @@ ink_rwlock_rdlock(ink_rwlock *rw)
 int
 ink_rwlock_wrlock(ink_rwlock *rw)
 {
-  int result;
-
   if (rw->rw_magic != RW_MAGIC)
     return EINVAL;
 
-  if ((result = ink_mutex_acquire(&rw->rw_mutex)) != 0)
-    return result;
+  ink_mutex_acquire(&rw->rw_mutex);
 
   while (rw->rw_refcount != 0) {
     rw->rw_nwaitwriters++;
@@ -133,20 +121,18 @@ ink_rwlock_wrlock(ink_rwlock *rw)
 int
 ink_rwlock_unlock(ink_rwlock *rw)
 {
-  int result;
-
   if (rw->rw_magic != RW_MAGIC)
     return EINVAL;
 
-  if ((result = ink_mutex_acquire(&rw->rw_mutex)) != 0)
-    return result;
+  ink_mutex_acquire(&rw->rw_mutex);
 
-  if (rw->rw_refcount > 0)
+  if (rw->rw_refcount > 0) {
     rw->rw_refcount--; /* releasing a reader */
-  else if (rw->rw_refcount == -1)
+  } else if (rw->rw_refcount == -1) {
     rw->rw_refcount = 0; /* releasing a reader */
-  else
-    ink_release_assert("invalid rw_refcount!");
+  } else {
+    ink_abort("invalid refcount %d on ink_rwlock %p", rw->rw_refcount, rw);
+  }
 
   /* give preference to waiting writers over waiting readers */
   if (rw->rw_nwaitwriters > 0) {
diff --git a/lib/ts/ink_thread.h b/lib/ts/ink_thread.h
index 3e2519d..a5baffd 100644
--- a/lib/ts/ink_thread.h
+++ b/lib/ts/ink_thread.h
@@ -244,6 +244,7 @@ ink_cond_wait(ink_cond *cp, ink_mutex *mp)
 {
   ink_assert(pthread_cond_wait(cp, mp) == 0);
 }
+
 static inline int
 ink_cond_timedwait(ink_cond *cp, ink_mutex *mp, ink_timestruc *t)
 {
diff --git a/mgmt/api/EventControlMain.cc b/mgmt/api/EventControlMain.cc
index d86ce58..645262f 100644
--- a/mgmt/api/EventControlMain.cc
+++ b/mgmt/api/EventControlMain.cc
@@ -122,13 +122,7 @@ remove_event_client(EventClientT *client, InkHashTable *table)
 TSMgmtError
 init_mgmt_events()
 {
-  int ret;
-
-  ret = ink_mutex_init(&mgmt_events_lock, "mgmt_event_notice");
-
-  if (ret) {
-    return TS_ERR_SYS_CALL;
-  }
+  ink_mutex_init(&mgmt_events_lock, "mgmt_event_notice");
 
   // initialize queue
   mgmt_events = create_queue();
diff --git a/proxy/InkAPITest.cc b/proxy/InkAPITest.cc
index 0553b44..47f34ce 100644
--- a/proxy/InkAPITest.cc
+++ b/proxy/InkAPITest.cc
@@ -1279,6 +1279,11 @@ REGRESSION_TEST(SDK_API_TSThread)(RegressionTest *test, int /* atype
ATS_UNUSED
   } else {
     SDK_RPRINT(test, "TSThreadCreate", "TestCase1", TC_PASS, "ok");
   }
+
+  if (created_thread != nullptr) {
+    TSThreadWait(created_thread);
+    TSThreadDestroy(created_thread);
+  }
 }
 
 //////////////////////////////////////////////
diff --git a/proxy/InkIOCoreAPI.cc b/proxy/InkIOCoreAPI.cc
index ec18570..237b055 100644
--- a/proxy/InkIOCoreAPI.cc
+++ b/proxy/InkIOCoreAPI.cc
@@ -96,22 +96,43 @@ TSReturnCode sdk_sanity_check_null_ptr(void *ptr);
 //
 ////////////////////////////////////////////////////////////////////
 struct INKThreadInternal : public EThread {
-  INKThreadInternal() : EThread(DEDICATED, -1) {}
-  TSThreadFunc func;
-  void *data;
+  INKThreadInternal() : EThread(DEDICATED, -1)
+  {
+    ink_mutex_init(&completion.lock, nullptr);
+    ink_cond_init(&completion.signal);
+  }
+
+  ~INKThreadInternal()
+  {
+    ink_mutex_destroy(&completion.lock);
+    ink_cond_destroy(&completion.signal);
+  }
+
+  TSThreadFunc func = nullptr;
+  void *data        = nullptr;
+
+  struct {
+    ink_mutex lock;
+    ink_cond signal;
+    bool done;
+  } completion;
 };
 
 static void *
 ink_thread_trampoline(void *data)
 {
-  INKThreadInternal *thread;
   void *retval;
+  INKThreadInternal *ithread = (INKThreadInternal *)data;
 
-  thread = (INKThreadInternal *)data;
-  thread->set_specific();
-  retval = thread->func(thread->data);
-  delete thread;
+  ithread->set_specific();
+  retval = ithread->func(ithread->data);
+
+  ink_mutex_acquire(&ithread->completion.lock);
+
+  ithread->completion.done = true;
+  ink_cond_broadcast(&ithread->completion.signal);
 
+  ink_mutex_release(&ithread->completion.lock);
   return retval;
 }
 
@@ -126,6 +147,7 @@ TSThreadCreate(TSThreadFunc func, void *data)
   thread = new INKThreadInternal;
 
   ink_assert(thread->event_types == 0);
+  ink_assert(thread->mutex->thread_holding == thread);
 
   thread->func = func;
   thread->data = data;
@@ -137,6 +159,27 @@ TSThreadCreate(TSThreadFunc func, void *data)
   return (TSThread)thread;
 }
 
+// Wait for a thread to complete. When a thread calls TSThreadCreate,
+// it becomes the owner of the thread's mutex. Since only the thread
+// that locked a mutex should be allowed to unlock it (a condition
+// that is enforced for PTHREAD_MUTEX_ERRORCHECK), if the application
+// needs to delete the thread, it must first wait for the thread to
+// complete.
+void
+TSThreadWait(TSThread thread)
+{
+  sdk_assert(sdk_sanity_check_iocore_structure(thread) == TS_SUCCESS);
+  INKThreadInternal *ithread = (INKThreadInternal *)thread;
+
+  ink_mutex_acquire(&ithread->completion.lock);
+
+  if (ithread->completion.done == false) {
+    ink_cond_wait(&ithread->completion.signal, &ithread->completion.lock);
+  }
+
+  ink_mutex_release(&ithread->completion.lock);
+}
+
 TSThread
 TSThreadInit()
 {
@@ -161,6 +204,17 @@ TSThreadDestroy(TSThread thread)
   sdk_assert(sdk_sanity_check_iocore_structure(thread) == TS_SUCCESS);
 
   INKThreadInternal *ithread = (INKThreadInternal *)thread;
+
+  // The thread must be destroyed by the same thread that created
+  // it because that thread is holding the thread mutex.
+  ink_release_assert(ithread->mutex->thread_holding == ithread);
+
+  // If this thread was created by TSThreadCreate() rather than
+  // TSThreadInit, then we must not destroy it before it's done.
+  if (ithread->func) {
+    ink_release_assert(ithread->completion.done == true);
+  }
+
   delete ithread;
 }
 
diff --git a/proxy/api/ts/ts.h b/proxy/api/ts/ts.h
index 96f47a5..ed7863e 100644
--- a/proxy/api/ts/ts.h
+++ b/proxy/api/ts/ts.h
@@ -1120,6 +1120,7 @@ tsapi const char *TSHttpHdrReasonLookup(TSHttpStatus status);
 tsapi TSThread TSThreadCreate(TSThreadFunc func, void *data);
 tsapi TSThread TSThreadInit(void);
 tsapi void TSThreadDestroy(TSThread thread);
+tsapi void TSThreadWait(TSThread thread);
 tsapi TSThread TSThreadSelf(void);
 
 /* --------------------------------------------------------------------------

-- 
To stop receiving notification emails like this one, please contact
"commits@trafficserver.apache.org" <commits@trafficserver.apache.org>.

Mime
View raw message