From 494ee2aa8d063f3159f3980832808872097286d6 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Fri, 10 Feb 2017 11:43:57 +0100 Subject: QThreadPool: supersede cancel() with tryTake() The cancel() function added in 5b11e43e for Qt 5.5 suffers from a number of problems: First, if runnable->autoDelete() is true, then the function suffers from the ABA problem (see documentation written for trytake()). Second, if runnable->autoDelete() is false, due to cancel() throwing away crucial information instead of returning it, the caller cannot know whether the runnable was canceled (and thus has to be deleted), wasn't found or is currently executing (and thus mustn't be deleted), or has finished executing (and can be used to extract the result). Deprecate this dangerous API and replace it with the much more useful Private::stealRunnable(), promoted to public API and renamed to tryTake() for consistency with the rest of Qt. Described the various caveats in the function's documentation. [ChangeLog][QtCore][QThreadPool] The cancel() function suffers from several subtle issues and has been replaced with a new tryTake() function. Change-Id: I93125935614087efa24b3e3969dd6718aeabaa4f Reviewed-by: Edward Welbourne --- src/corelib/thread/qthreadpool.cpp | 50 +++++++++++++++++++++++++------------- src/corelib/thread/qthreadpool.h | 5 ++++ src/corelib/thread/qthreadpool_p.h | 1 - 3 files changed, 38 insertions(+), 18 deletions(-) (limited to 'src/corelib/thread') diff --git a/src/corelib/thread/qthreadpool.cpp b/src/corelib/thread/qthreadpool.cpp index 2ecf8f729a..e45aaec103 100644 --- a/src/corelib/thread/qthreadpool.cpp +++ b/src/corelib/thread/qthreadpool.cpp @@ -316,22 +316,39 @@ void QThreadPoolPrivate::clear() } /*! - \internal - Searches for \a runnable in the queue, removes it from the queue and - returns \c true if it was found in the queue + \since 5.9 + + Attempts to remove the specified \a runnable from the queue if it is not yet started. + If the runnable had not been started, returns \c true, and ownership of \a runnable + is transferred to the caller (even when \c{runnable->autoDelete() == true}). + Otherwise returns \c false. + + \note If \c{runnable->autoDelete() == true}, this function may remove the wrong + runnable. This is known as the \l{https://en.wikipedia.org/wiki/ABA_problem}{ABA problem}: + the original \a runnable may already have executed and has since been deleted. + The memory is re-used for another runnable, which then gets removed instead of + the intended one. For this reason, we recommend calling this function only for + runnables that are not auto-deleting. + + \sa start(), QRunnable::autoDelete() */ -bool QThreadPoolPrivate::stealRunnable(QRunnable *runnable) +bool QThreadPool::tryTake(QRunnable *runnable) { + Q_D(QThreadPool); + if (runnable == 0) return false; { - QMutexLocker locker(&mutex); - QVector >::iterator it = queue.begin(); - QVector >::iterator end = queue.end(); + QMutexLocker locker(&d->mutex); + + auto it = d->queue.begin(); + auto end = d->queue.end(); while (it != end) { if (it->first == runnable) { - queue.erase(it); + d->queue.erase(it); + if (runnable->autoDelete()) + --runnable->ref; // undo ++ref in start() return true; } ++it; @@ -349,10 +366,10 @@ bool QThreadPoolPrivate::stealRunnable(QRunnable *runnable) */ void QThreadPoolPrivate::stealAndRunRunnable(QRunnable *runnable) { - if (!stealRunnable(runnable)) + Q_Q(QThreadPool); + if (!q->tryTake(runnable)) return; - const bool autoDelete = runnable->autoDelete(); - bool del = autoDelete && !--runnable->ref; + const bool del = runnable->autoDelete() && !runnable->ref; // tryTake already deref'ed runnable->run(); @@ -642,24 +659,23 @@ void QThreadPool::clear() d->clear(); } +#if QT_DEPRECATED_SINCE(5, 9) /*! \since 5.5 + \obsolete use tryTake() instead, but note the different deletion rules. Removes the specified \a runnable from the queue if it is not yet started. The runnables for which \l{QRunnable::autoDelete()}{runnable->autoDelete()} returns \c true are deleted. - \sa start() + \sa start(), tryTake() */ void QThreadPool::cancel(QRunnable *runnable) { - Q_D(QThreadPool); - if (!d->stealRunnable(runnable)) - return; - if (runnable->autoDelete() && !--runnable->ref) { + if (tryTake(runnable) && runnable->autoDelete() && !runnable->ref) // tryTake already deref'ed delete runnable; - } } +#endif QT_END_NAMESPACE diff --git a/src/corelib/thread/qthreadpool.h b/src/corelib/thread/qthreadpool.h index 0ad63c5ac3..74a8c28fc8 100644 --- a/src/corelib/thread/qthreadpool.h +++ b/src/corelib/thread/qthreadpool.h @@ -83,7 +83,12 @@ public: bool waitForDone(int msecs = -1); void clear(); + +#if QT_DEPRECATED_SINCE(5, 9) + QT_DEPRECATED_X("use tryTake(), but note the different deletion rules") void cancel(QRunnable *runnable); +#endif + bool tryTake(QRunnable *runnable) Q_REQUIRED_RESULT; }; QT_END_NAMESPACE diff --git a/src/corelib/thread/qthreadpool_p.h b/src/corelib/thread/qthreadpool_p.h index ea8127efef..4a9f9e5cfa 100644 --- a/src/corelib/thread/qthreadpool_p.h +++ b/src/corelib/thread/qthreadpool_p.h @@ -82,7 +82,6 @@ public: void reset(); bool waitForDone(int msecs); void clear(); - bool stealRunnable(QRunnable *runnable); void stealAndRunRunnable(QRunnable *runnable); mutable QMutex mutex; -- cgit v1.2.3