From 6bd271cf74d6d57816531f688c82c51d29f1be91 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Wed, 23 Oct 2024 10:11:30 -0700 Subject: [PATCH] QThread: avoid unlock/lock/unlock in ~QThread if state is Finishing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a corner-case scenario but valid because we tell users they can destroy the QThread object right after finished() has been emitted. But emitting finished() does not mean the launched thread has actually exited: it may still be in Finishing state for an arbitrarily long time. Completely aside from what else may run from other libraries, we only destroy QThreadStorage and the thread's event dispatcher after finished() has been emitted. This commit avoids the unnecessary mutex unlocking in the destructor, then QThread::wait() locking again, only to unlock yet again so that it can perform the necessary low-level wait calls. The same for the return path: wait() locked again to check the state, then unlocked, only for the destructor to lock again. Now, QThreadPrivate::wait() is responsible for returning with a locked mutex. Pick-to: 6.8 Change-Id: I87adffb89f275accea18fffd6b4293861ea7cf39 Reviewed-by: Mårten Nordheim Reviewed-by: Edward Welbourne --- src/corelib/thread/qthread.cpp | 7 ++----- src/corelib/thread/qthread_p.h | 2 ++ src/corelib/thread/qthread_unix.cpp | 11 +++++++++++ src/corelib/thread/qthread_win.cpp | 8 ++++++++ 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/corelib/thread/qthread.cpp b/src/corelib/thread/qthread.cpp index 870689f4d0c..9d42c5dc02a 100644 --- a/src/corelib/thread/qthread.cpp +++ b/src/corelib/thread/qthread.cpp @@ -483,11 +483,8 @@ QThread::~QThread() Q_D(QThread); { QMutexLocker locker(&d->mutex); - if (d->threadState == QThreadPrivate::Finishing) { - locker.unlock(); - wait(); - locker.relock(); - } + if (d->threadState == QThreadPrivate::Finishing) + d->wait(locker, QDeadlineTimer::Forever); if (d->threadState == QThreadPrivate::Running && !d->data->isAdopted) qFatal("QThread: Destroyed while thread '%ls' is still running", qUtf16Printable(objectName())); diff --git a/src/corelib/thread/qthread_p.h b/src/corelib/thread/qthread_p.h index 5a7b241cd04..ada010dfefe 100644 --- a/src/corelib/thread/qthread_p.h +++ b/src/corelib/thread/qthread_p.h @@ -204,6 +204,8 @@ public: uint stackSize = 0; std::underlying_type_t priority = QThread::InheritPriority; + bool wait(QMutexLocker &locker, QDeadlineTimer deadline); + #ifdef Q_OS_UNIX QWaitCondition thread_done; diff --git a/src/corelib/thread/qthread_unix.cpp b/src/corelib/thread/qthread_unix.cpp index a540687dcd1..90505ba51ec 100644 --- a/src/corelib/thread/qthread_unix.cpp +++ b/src/corelib/thread/qthread_unix.cpp @@ -822,6 +822,17 @@ bool QThread::wait(QDeadlineTimer deadline) if (d->threadState == QThreadPrivate::NotStarted) return true; + if (d->threadState == QThreadPrivate::Finished) + return true; + + return d->wait(locker, deadline); +} + +bool QThreadPrivate::wait(QMutexLocker &locker, QDeadlineTimer deadline) +{ + Q_ASSERT(threadState != QThreadPrivate::Finished); + Q_ASSERT(locker.isLocked()); + QThreadPrivate *d = this; while (d->threadState != QThreadPrivate::Finished) { if (!d->thread_done.wait(locker.mutex(), deadline)) diff --git a/src/corelib/thread/qthread_win.cpp b/src/corelib/thread/qthread_win.cpp index 6d8d630f2cf..3fb07db62da 100644 --- a/src/corelib/thread/qthread_win.cpp +++ b/src/corelib/thread/qthread_win.cpp @@ -488,6 +488,14 @@ bool QThread::wait(QDeadlineTimer deadline) } if (d->threadState == QThreadPrivate::NotStarted || d->threadState == QThreadPrivate::Finished) return true; + return d->wait(locker, deadline); +} + +bool QThreadPrivate::wait(QMutexLocker &locker, QDeadlineTimer deadline) +{ + Q_ASSERT(threadState != QThreadPrivate::Finished); + Q_ASSERT(locker.isLocked()); + QThreadPrivate *d = this; ++d->waiters; locker.mutex()->unlock();