From 605647f2b8a6c8d4f700732a7fcdb1dec3459231 Mon Sep 17 00:00:00 2001
From: David Faure <david.faure@kdab.com>
Date: Fri, 21 Jun 2013 10:32:17 +0200
Subject: QThreadPool: fix counting of waiting threads

QTBUG-21051 has a testcase where activeThreadCount() could actually
end up at -1 (converted to an autotest in this commit).
The reason was: start() calls tryStart() which returns false due to
too many active threads (reserveThread() causes this), so it calls
enqueueTask() - which actually wakes up the waiting thread, but
it didn't decrement the number of waiting threads.

Note that tryStart() is "if I can grab a waiting thread, enqueue task and wake it"
while start(), in case tryStart() fails, wants to "enqueue, and then if I can grab
a waiting thread, wake it". This is why enqueue shouldn't wake; waking must happen
only if we can grab a thread (d->waitingThreads > 0).

Task-number: QTBUG-21051
Backport from qtbase/dacf9961da86751a59da0e84bc943fe0d1c8d95b

Change-Id: I1e437da27b733a72b48ff1b6f2b78f81a7ed129b
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
---
 src/corelib/concurrent/qthreadpool.cpp     | 10 +++-
 tests/auto/qthreadpool/tst_qthreadpool.cpp | 74 ++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/src/corelib/concurrent/qthreadpool.cpp b/src/corelib/concurrent/qthreadpool.cpp
index 5435aef..26a5337 100644
--- a/src/corelib/concurrent/qthreadpool.cpp
+++ b/src/corelib/concurrent/qthreadpool.cpp
@@ -187,6 +187,7 @@ bool QThreadPoolPrivate::tryStart(QRunnable *task)
         // recycle an available thread
         --waitingThreads;
         enqueueTask(task);
+        runnableReady.wakeOne();
         return true;
     }
 
@@ -218,7 +219,6 @@ void QThreadPoolPrivate::enqueueTask(QRunnable *runnable, int priority)
     QList<QPair<QRunnable *, int> >::iterator at =
         qUpperBound(queue.begin(), queue.end(), priority);
     queue.insert(at, qMakePair(runnable, priority));
-    runnableReady.wakeOne();
 }
 
 int QThreadPoolPrivate::activeThreadCount() const
@@ -471,8 +471,14 @@ void QThreadPool::start(QRunnable *runnable, int priority)
 
     Q_D(QThreadPool);
     QMutexLocker locker(&d->mutex);
-    if (!d->tryStart(runnable))
+    if (!d->tryStart(runnable)) {
         d->enqueueTask(runnable, priority);
+
+        if (d->waitingThreads > 0) {
+            --d->waitingThreads;
+            d->runnableReady.wakeOne();
+        }
+    }
 }
 
 /*!
diff --git a/tests/auto/qthreadpool/tst_qthreadpool.cpp b/tests/auto/qthreadpool/tst_qthreadpool.cpp
index 5d134ab..61343c1 100644
--- a/tests/auto/qthreadpool/tst_qthreadpool.cpp
+++ b/tests/auto/qthreadpool/tst_qthreadpool.cpp
@@ -44,6 +44,24 @@
 #include <qstring.h>
 #include <qmutex.h>
 
+// From Qt5
+
+#define QTRY_COMPARE_WITH_TIMEOUT(__expr, __expected, __timeout) \
+    do { \
+            const int __step = 50; \
+            const int __timeoutValue = __timeout; \
+            if ((__expr) != (__expected)) { \
+                        QTest::qWait(0); \
+                    } \
+            for (int __i = 0; __i < __timeoutValue && ((__expr) != (__expected)); __i+=__step) { \
+                        QTest::qWait(__step); \
+                    } \
+            QCOMPARE(__expr, __expected); \
+    } while (0)
+
+#define QTRY_COMPARE(__expr, __expected) QTRY_COMPARE_WITH_TIMEOUT(__expr, __expected, 5000)
+
+
 typedef void (*FunctionPointer)();
 
 class FunctionPointerTask : public QRunnable
@@ -90,6 +108,7 @@ private slots:
     void reserveThread();
     void releaseThread_data();
     void releaseThread();
+    void reserveAndStart();
     void start();
     void tryStart();
     void tryStartPeakThreadCount();
@@ -647,6 +666,61 @@ void tst_QThreadPool::releaseThread()
     threadpool->setMaxThreadCount(savedLimit);
 }
 
+void tst_QThreadPool::reserveAndStart() // QTBUG-21051
+{
+    class WaitingTask : public QRunnable
+    {
+    public:
+        QAtomicInt count;
+        QSemaphore waitForStarted;
+
+        WaitingTask() { setAutoDelete(false); }
+
+        void run()
+        {
+            count.ref();
+            waitForStarted.release();
+        }
+    };
+
+    // Set up
+    QThreadPool *threadpool = QThreadPool::globalInstance();
+    int savedLimit = threadpool->maxThreadCount();
+    threadpool->setMaxThreadCount(1);
+    QCOMPARE(threadpool->activeThreadCount(), 0);
+
+    // reserve
+    threadpool->reserveThread();
+    QCOMPARE(threadpool->activeThreadCount(), 1);
+
+    // start a task, to get a running thread
+    WaitingTask *task = new WaitingTask;
+    threadpool->start(task);
+    QCOMPARE(threadpool->activeThreadCount(), 2);
+    task->waitForStarted.acquire();
+    QTRY_COMPARE(int(task->count), 1);
+    QTRY_COMPARE(threadpool->activeThreadCount(), 1);
+
+    // now the thread is waiting, but tryStart() will fail since activeThreadCount() >= maxThreadCount()
+    QVERIFY(!threadpool->tryStart(task));
+    QTRY_COMPARE(threadpool->activeThreadCount(), 1);
+
+    // start() will therefore do a failing tryStart(), followed by enqueueTask()
+    // which will actually wake up the waiting thread.
+    threadpool->start(task);
+    QTRY_COMPARE(threadpool->activeThreadCount(), 2);
+    task->waitForStarted.acquire();
+    QTRY_COMPARE(int(task->count), 2);
+    QTRY_COMPARE(threadpool->activeThreadCount(), 1);
+
+    threadpool->releaseThread();
+    QTRY_COMPARE(threadpool->activeThreadCount(), 0);
+
+    delete task;
+
+    threadpool->setMaxThreadCount(savedLimit);
+}
+
 QAtomicInt count;
 class CountingRunnable : public QRunnable
 {
-- 
cgit v0.12