[THRIFT-3932] C++ ThreadManager has a rare termination race

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 0.10.0
• Component/s: C++ - Library
• Patch Info: Patch Available
• Assignee: James E. King III
• Reporter: Buğra Gedik
• Created: 20/Sep/16 07:45
• Updated: 11/Jan/18 12:43
• Resolved: 12/Nov/16 20:22

Description

ThreadManger::join calls stopImpl(true), which in turn calls removeWorker(workerCount_);. The latter waits until while (workerCount_ != workerMaxCount_). Within the run method of the workers, the last thread that detects workerCount_ == workerMaxCount_ notifies removeWorker. The run method has the following additional code that is executed at the very end:

    {
      Synchronized s(manager_->workerMonitor_);
      manager_->deadWorkers_.insert(this->thread());
      if (notifyManager) {
        manager_->workerMonitor_.notify();
      }
    }

This is an independent synchronized block. Now assume 2 threads. One of them has notifyManager=true as it detected the workerCount_ == workerMaxCount_ condition earlier. It is possible that this thread gets to execute the above code block first, ThreadManager's removeWorker method unblocks, and eventually ThreadManager's join returns and the object is destructed. When the other thread reaches the synchronized block above, it will crash, as the manager is not around anymore.

Besides, ThreadManager never joins its threads.

Attached is a patch.

Resolution Details

A fair number of things were improved with this pull request:

1. There are three monitors used for signaling; two of them use the same mutex and then the workerMonitor is using its own. I made all three use the same mutex.
2. I replaced all calls to Synchronize with Guard on the mutex_ so it is a bit clearer that there is one lock protecting everything.
3. The worker run loop expires tasks as it encounters them. Now we only call removeExpiredTasks if there's no room during add.
4. I found that when worker threads are removed they are not joined; if a thread is not detached we now join those threads.
5. I added better documentation and simplified the Worker::run() a little so it is easier to understand, but the logic is the same. idle_ was not used and removed.
6. I found that remove(Task) simply wasn't implemented at all! so I added code for that and tests.
7. Release mode tests of thread manager would not fail on error because they use assert() to verify results! If we only do release builds in CI, these tests may have been silently failing for a long time.
8. ThreadManager::join() is unnecessary, as stop() will join worker threads (as will removeWorker) depending on the ThreadFactory's detached disposition.
9. Cleaned up some technical debt from the TServerFramework refactoring:
10. Cleaned up some things in the different platform thread factories. Some abstractions were unnecessary.
11. Reduced overall time on concurrency test to about 30 seconds on my dev system while increasing the reliability and improving test coverage significantly. I ran it 100 times in a loop.
12. Added tests for most of the ThreadManager APIs.
13. Tested with posix, std, and boost threads on linux.
14. Tested on Windows.
15. Fixed a math error in time calculation that was present on Windows (with VS2010) which made expiring tasks impossible (unit test proved the issue and the fix).
16. Re-enable unit tests in the appveyor build that are no longer unstable.

Issue Links

• blocks: THRIFT-3873 Fix various build warnings when using Visual Studio
• is duplicated by: THRIFT-4336 TThreadedServer mode maybe lock and can't work
• is duplicated by: THRIFT-487 ThreadManagerTests::blockTest errors
• is duplicated by: THRIFT-2755 ThreadSanitizer reports data race in ThreadManager::Impl::addWorker
• is related to: THRIFT-3233 Fix C++ ThreadManager::Impl::removeWorker worker join
• is required by: THRIFT-3777 processor and concurrency tests in C++ are not part of the standard test suite and are failing
• links to: GitHub Pull Request #1103