MB-19253: Fix race in void ExecutorPool::doWorkerStat

As reported by ThreadSanitizer (see below), there is a race between
setting the current task associated with a ExecutorThread and reading
the name of that thread.

Unfortunately there doesn't seem to be a straightforward way to solve
this without adding a new mutex; currentTask (the variable the race is
on) is a SingleThreadedRCPtr, which is non-trivial to make thread-safe
(i.e. atomic). I did consider changing currenTask (and all other
ExTask variables) to be a std::shared_ptr as in C++11 this has support
for updating atomically, however the support in mainstream compilers
apparently isn't quite there yet.

Therefore I've just added a mutex to guard currentTask.

ThreadSanitizer report:

WARNING: ThreadSanitizer: data race (pid=27332)
  Read of size 8 at 0x7d340000c8f0 by main thread (mutexes: write M19366):
    #0 ExecutorThread::getTaskableName() const /home/couchbase/couchbase/ep-engine/src/atomic.h:309 (ep.so+0x0000000e6178)
    #1 EventuallyPersistentEngine::getStats(void const*, char const*, int, void (*)(char const*, unsigned short, char const*, unsigned int, void const*)) /home/couchbase/couchbase/ep-engine/src/ep_engine.cc:4346 (ep.so+0x0000000bc4dd)
    #2 EvpGetStats(engine_interface*, void const*, char const*, int, void (*)(char const*, unsigned short, char const*, unsigned int, void const*)) /home/couchbase/couchbase/ep-engine/src/ep_engine.cc:213 (ep.so+0x0000000ab49e)
    #3 mock_get_stats(engine_interface*, void const*, char const*, int, void (*)(char const*, unsigned short, char const*, unsigned int, void const*)) /home/couchbase/couchbase/memcached/programs/engine_testapp/engine_testapp.cc:239 (engine_testapp+0x0000004c54ad)
    #4 test_worker_stats(engine_interface*, engine_interface_v1*) /home/couchbase/couchbase/ep-engine/tests/ep_testsuite.cc:8901 (ep_testsuite.so+0x000000039768)
    #5 execute_test(test, char const*, char const*) /home/couchbase/couchbase/memcached/programs/engine_testapp/engine_testapp.cc:1090 (engine_testapp+0x0000004c40b2)
    #6 __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 (libc.so.6+0x00000002176c)

  Previous write of size 8 at 0x7d340000c8f0 by thread T5:
    #0 ExecutorThread::run() /home/couchbase/couchbase/ep-engine/src/atomic.h:322 (ep.so+0x0000000e9906)
    #1 launch_executor_thread(void*) /home/couchbase/couchbase/ep-engine/src/executorthread.cc:33 (ep.so+0x0000000e9795)
    #2 platform_thread_wrap /home/couchbase/.ccache/tmp/cb_pthread.tmp.00b591814417.18511.i (libplatform.so.0.1.0+0x000000003d91)

Change-Id: Id02f7a98b40b952a415cf9027a8f2243af38fc4d
Reviewed-on: http://review.couchbase.org/62973
Well-Formed: buildbot <[email protected]>
Reviewed-by: Will Gardner <[email protected]>
Tested-by: buildbot <[email protected]>

Author: daverigby

src/executorthread.cc

@@ -70,7 +70,10 @@ void ExecutorThread::run() {
     LOG(EXTENSION_LOG_DEBUG, "Thread %s running..", getName().c_str());
 
     for (uint8_t tick = 1;; tick++) {
-        currentTask.reset();
+        {
+            LockHolder lh(currentTaskMutex);
+            currentTask.reset();
+        }
         if (state != EXECUTOR_RUNNING) {
             break;
         }
@@ -155,6 +158,11 @@ void ExecutorThread::run() {
     state = EXECUTOR_DEAD;
 }
 
+void ExecutorThread::setCurrentTask(ExTask newTask) {
+    LockHolder lh(currentTaskMutex);
+    currentTask = newTask;
+}
+
 void ExecutorThread::addLogEntry(const std::string &desc,
                                  const task_type_t taskType,
                                  const hrtime_t runtime,

src/executorthread.h

@@ -85,9 +85,13 @@ class ExecutorThread {
 
     void wake(ExTask &task);
 
+    // Changes this threads' current task to the specified task
+    void setCurrentTask(ExTask newTask);
+
     const std::string& getName() const { return name; }
 
-    const std::string getTaskName() const {
+    const std::string getTaskName() {
+        LockHolder lh(currentTaskMutex);
         if (currentTask) {
             return currentTask->getDescription();
         } else {
@@ -129,7 +133,10 @@ class ExecutorThread {
     hrtime_t waketime; // set to the earliest
 
     hrtime_t taskStart;
+
+    Mutex currentTaskMutex; // Protects currentTask
     ExTask currentTask;
+
     task_type_t curTaskType;
 
     Mutex logMutex;

src/taskqueue.cc

@@ -126,7 +126,7 @@ bool TaskQueue::_fetchNextTask(ExecutorThread &t, bool toSleep) {
     }
 
     if (!readyQueue.empty() && readyQueue.top()->isdead()) {
-        t.currentTask = _popReadyTask(); // clean out dead tasks first
+        t.setCurrentTask(_popReadyTask()); // clean out dead tasks first
         ret = true;
     } else if (!readyQueue.empty() || !pendingQueue.empty()) {
         t.curTaskType = manager->tryNewWork(queueType);
@@ -138,7 +138,7 @@ bool TaskQueue::_fetchNextTask(ExecutorThread &t, bool toSleep) {
             _checkPendingQueue();
 
             ExTask tid = _popReadyTask(); // and pop out the top task
-            t.currentTask = tid; // assign task to thread
+            t.setCurrentTask(tid);
             ret = true;
         } else if (!readyQueue.empty()) { // We hit limit on max # workers
             ExTask tid = _popReadyTask(); // that can work on current Q type!