Prevent deadlock in MThreadPool #1066
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
6 tasks
|
I think QThreadPool's implementation is better architected in that it has only one mutex in the QThreadPool instead of that one and one per pool thread and that the pool threads request from the QThreadPool a QRunnable to run instead of being given one by the thread pool; however, I don't plan on changing this. |
MPoolThread::m_lock is always locked except during the m_wait.wait() call and the calls to m_pool.NotifyAvailable() and m_pool.NotifyDone(). If m_doRun is false at the end of the infinite loop (only possible if MPoolThread::SetRunnable() and then MPoolThread::Shutdown() are called before MPoolThread::run() acquires the lock, since m_doRun will be false and m_runnable will not be nullptr when m_runnable is tested in the loop) and the else branch is removed, the next iteration will skip the wait and perform the same operations since the lock is never released. Also, m_doRun is never set back to true once it is false and m_runnable is never set to a non-nullptr value if m_doRun is false. This reveals m_runnable != nullptr is the true loop condition (and also that the QRunnable will be run after Shutdown() is called). The test for m_doRun is preserved so m_pool.NotifyAvailable() is still not called when m_doRun is false. The code is then reordered so the loop condition can be used. Since the lock was released and another thread could have been waiting in MPoolThread::SetRunnable() or MPoolThread::Shutdown(), we need to check m_runnable == nullptr in addition to m_doRun before waiting.
It was not used when it was added, so it was probably never used.
It deadlocks if any thread is running and m_reserved is true since the thread will call m_pool.ReleaseThread() which will attempt to lock MThreadPool::m_priv::m_lock, but MThreadPool::Stop() holds that lock and is waiting to acquire the lock of MPoolThread::m_lock in MPoolThread::Shutdown() which was not released. This deadlock can be triggered in mythfrontend by using http://localhost:6551/MythFE/GetRemote and then exiting within 10 seconds. The shutdown is still delayed by the 10 seconds that HttpWorker::run() is waiting.
m_runnable will not change since it is not nullptr. This allows Shutdown() to be executed while the thread is in m_runnable->run(). HttpWorker::run() still delays shutdown by 10 seconds, but all of the calls to MPoolThread::Shutdown() in MThreadPool::Stop() are immediately executed.
The removed else branch was never executed since bTimeout would have been true and the loop exited earlier. The HttpWorker threads now shut down within 2 milliseconds. However, ~MythContext() still waits for about 3 seconds for threads to exit.
201aca6 to
b50024c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@bennettpeter This resolves the shutdown issue mentioned in #1044
The deadlock can be triggered in mythfrontend by using http://localhost:6551/MythFE/GetRemote and then exiting within 10 seconds.
For a version with additional logging I used to diagnose this, see https://github.com/ulmus-scott/mythtv/tree/upnp_shutdown_log
Checklist