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 <edward.welbourne@qt.io>

1 files changed, 92 insertions, 0 deletions

diff --git a/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp b/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp
index 5b7242fddb..fdc4ecb5c8 100644
--- a/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp
+++ b/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp
@@ -89,6 +89,7 @@ private slots:
     void waitForDone();
     void clear();
     void cancel();
+    void tryTake();
     void waitForDoneTimeout();
     void destroyingWaitsForTasksToFinish();
     void stressTest();
@@ -1042,6 +1043,97 @@ void tst_QThreadPool::cancel()
     delete runnables[runs-1];
 }
 
+void tst_QThreadPool::tryTake()
+{
+    QSemaphore sem(0);
+    QSemaphore startedThreads(0);
+
+    class SemaphoreReleaser
+    {
+        QSemaphore &sem;
+        int n;
+        Q_DISABLE_COPY(SemaphoreReleaser)
+    public:
+        explicit SemaphoreReleaser(QSemaphore &sem, int n)
+            : sem(sem), n(n) {}
+
+        ~SemaphoreReleaser()
+        {
+            sem.release(n);
+        }
+    };
+
+    class BlockingRunnable : public QRunnable
+    {
+    public:
+        QSemaphore &sem;
+        QSemaphore &startedThreads;
+        QAtomicInt &dtorCounter;
+        QAtomicInt &runCounter;
+        int dummy;
+
+        explicit BlockingRunnable(QSemaphore &s, QSemaphore &started, QAtomicInt &c, QAtomicInt &r)
+            : sem(s), startedThreads(started), dtorCounter(c), runCounter(r) {}
+
+        ~BlockingRunnable()
+        {
+            dtorCounter.fetchAndAddRelaxed(1);
+        }
+
+        void run() override
+        {
+            startedThreads.release();
+            runCounter.fetchAndAddRelaxed(1);
+            sem.acquire();
+            count.ref();
+        }
+    };
+
+    enum {
+        MaxThreadCount = 3,
+        OverProvisioning = 2,
+        Runs = MaxThreadCount * OverProvisioning
+    };
+
+    QThreadPool threadPool;
+    threadPool.setMaxThreadCount(MaxThreadCount);
+    BlockingRunnable *runnables[Runs];
+
+    // ensure that the QThreadPool doesn't deadlock if any of the checks fail
+    // and cause an early return:
+    const SemaphoreReleaser semReleaser(sem, Runs);
+
+    count.store(0);
+    QAtomicInt dtorCounter = 0;
+    QAtomicInt runCounter = 0;
+    for (int i = 0; i < Runs; i++) {
+        runnables[i] = new BlockingRunnable(sem, startedThreads, dtorCounter, runCounter);
+        runnables[i]->setAutoDelete(i != 0 && i != Runs - 1); // one which will run and one which will not
+        QVERIFY(!threadPool.tryTake(runnables[i])); // verify NOOP for jobs not in the queue
+        threadPool.start(runnables[i]);
+    }
+    // wait for all worker threads to have started up:
+    QVERIFY(startedThreads.tryAcquire(MaxThreadCount, 60*1000 /* 1min */));
+
+    for (int i = 0; i < MaxThreadCount; ++i) {
+        // check taking runnables doesn't work once they were started:
+        QVERIFY(!threadPool.tryTake(runnables[i]));
+    }
+    for (int i = MaxThreadCount; i < Runs ; ++i) {
+        QVERIFY(threadPool.tryTake(runnables[i]));
+        delete runnables[i];
+    }
+
+    runnables[0]->dummy = 0; // valgrind will catch this if tryTake() is crazy enough to delete currently running jobs
+    QCOMPARE(dtorCounter.load(), int(Runs - MaxThreadCount));
+    sem.release(MaxThreadCount);
+    threadPool.waitForDone();
+    QCOMPARE(runCounter.load(), int(MaxThreadCount));
+    QCOMPARE(count.load(), int(MaxThreadCount));
+    QCOMPARE(dtorCounter.load(), int(Runs - 1));
+    delete runnables[0]; // if the pool deletes them then we'll get double-free crash
+}
+
 void tst_QThreadPool::destroyingWaitsForTasksToFinish()
 {
     QTime total, pass;