From 6c4d75a485904e1b06964aea953479c167b507e8 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Sat, 11 Feb 2017 10:50:09 +0100 Subject: tst_QSemaphore: avoid deadlock on test failures When one of the QCOMPAREs in Consumer::run() fails, the consumer returns early, leaving the producer deadlocked in a QSemaphore's acquire() call. Change these to tryAcquire() with a large timeout, so the producer, too, eventually leaves run(). Change-Id: I7421d43305decd4754e09c8e092363594d1be06b Reviewed-by: David Faure --- tests/auto/corelib/thread/qsemaphore/tst_qsemaphore.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'tests/auto/corelib/thread') diff --git a/tests/auto/corelib/thread/qsemaphore/tst_qsemaphore.cpp b/tests/auto/corelib/thread/qsemaphore/tst_qsemaphore.cpp index 8597bf1a6d..e984618bdb 100644 --- a/tests/auto/corelib/thread/qsemaphore/tst_qsemaphore.cpp +++ b/tests/auto/corelib/thread/qsemaphore/tst_qsemaphore.cpp @@ -367,16 +367,18 @@ public: void run(); }; +static const int Timeout = 60 * 1000; // 1min + void Producer::run() { for (int i = 0; i < DataSize; ++i) { - freeSpace.acquire(); + QVERIFY(freeSpace.tryAcquire(1, Timeout)); buffer[i % BufferSize] = alphabet[i % AlphabetSize]; usedSpace.release(); } for (int i = 0; i < DataSize; ++i) { if ((i % ProducerChunkSize) == 0) - freeSpace.acquire(ProducerChunkSize); + QVERIFY(freeSpace.tryAcquire(ProducerChunkSize, Timeout)); buffer[i % BufferSize] = alphabet[i % AlphabetSize]; if ((i % ProducerChunkSize) == (ProducerChunkSize - 1)) usedSpace.release(ProducerChunkSize); -- cgit v1.2.3 From dcf74bdec83ba450b2016081339f48ac7469e1f5 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Fri, 10 Feb 2017 10:00:53 +0100 Subject: Fix UB (data race) in tst_QThreadPool::cancel() Manipulating a simple int from multiple threads is a data race, thus undefined behavior. Fix by using QAtomicInt and atomic operations instead. Change-Id: I5418bc260da57fe353a71b8e5c7c1c97adbe7597 Reviewed-by: David Faure --- .../corelib/thread/qthreadpool/tst_qthreadpool.cpp | 27 ++++++++++++++-------- 1 file changed, 17 insertions(+), 10 deletions(-) (limited to 'tests/auto/corelib/thread') diff --git a/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp b/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp index 3bc3f8a759..49e1177e79 100644 --- a/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp +++ b/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp @@ -962,14 +962,21 @@ void tst_QThreadPool::cancel() { public: QSemaphore & sem; - int & dtorCounter; - int & runCounter; + QAtomicInt &dtorCounter; + QAtomicInt &runCounter; int dummy; - BlockingRunnable(QSemaphore & s, int & c, int & r) : sem(s), dtorCounter(c), runCounter(r){} - ~BlockingRunnable(){dtorCounter++;} + + explicit BlockingRunnable(QSemaphore &s, QAtomicInt &c, QAtomicInt &r) + : sem(s), dtorCounter(c), runCounter(r){} + + ~BlockingRunnable() + { + dtorCounter.fetchAndAddRelaxed(1); + } + void run() { - runCounter++; + runCounter.fetchAndAddRelaxed(1); sem.acquire(); count.ref(); } @@ -981,8 +988,8 @@ void tst_QThreadPool::cancel() int runs = 2 * threadPool.maxThreadCount(); BlockingRunnablePtr* runnables = new BlockingRunnablePtr[runs]; count.store(0); - int dtorCounter = 0; - int runCounter = 0; + QAtomicInt dtorCounter = 0; + QAtomicInt runCounter = 0; for (int i = 0; i < runs; i++) { runnables[i] = new BlockingRunnable(sem, dtorCounter, runCounter); runnables[i]->setAutoDelete(i != 0 && i != (runs-1)); //one which will run and one which will not @@ -994,12 +1001,12 @@ void tst_QThreadPool::cancel() } runnables[0]->dummy = 0; //valgrind will catch this if cancel() is crazy enough to delete currently running jobs runnables[runs-1]->dummy = 0; - QCOMPARE(dtorCounter, runs-threadPool.maxThreadCount()-1); + QCOMPARE(dtorCounter.load(), runs - threadPool.maxThreadCount() - 1); sem.release(threadPool.maxThreadCount()); threadPool.waitForDone(); - QCOMPARE(runCounter, threadPool.maxThreadCount()); + QCOMPARE(runCounter.load(), threadPool.maxThreadCount()); QCOMPARE(count.load(), threadPool.maxThreadCount()); - QCOMPARE(dtorCounter, runs-2); + QCOMPARE(dtorCounter.load(), runs - 2); delete runnables[0]; //if the pool deletes them then we'll get double-free crash delete runnables[runs-1]; delete[] runnables; -- cgit v1.2.3 From 8087ea67b1457db5da5a641628a11a84a3a119f2 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Fri, 10 Feb 2017 10:06:59 +0100 Subject: tst_QThreadPool: simplify cancel() Instead of allocating a statically-sized array on the heap, use an automatic C array instead. Replace some magic numbers with named constants. Change-Id: I17d29a76a67c4a413453ac26a5dee8cd54a8a37d Reviewed-by: David Faure --- tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'tests/auto/corelib/thread') diff --git a/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp b/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp index 49e1177e79..dede0be170 100644 --- a/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp +++ b/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp @@ -981,12 +981,16 @@ void tst_QThreadPool::cancel() count.ref(); } }; - typedef BlockingRunnable* BlockingRunnablePtr; + + enum { + MaxThreadCount = 3, + OverProvisioning = 2, + runs = MaxThreadCount * OverProvisioning + }; QThreadPool threadPool; - threadPool.setMaxThreadCount(3); - int runs = 2 * threadPool.maxThreadCount(); - BlockingRunnablePtr* runnables = new BlockingRunnablePtr[runs]; + threadPool.setMaxThreadCount(MaxThreadCount); + BlockingRunnable *runnables[runs]; count.store(0); QAtomicInt dtorCounter = 0; QAtomicInt runCounter = 0; @@ -1009,7 +1013,6 @@ void tst_QThreadPool::cancel() QCOMPARE(dtorCounter.load(), runs - 2); delete runnables[0]; //if the pool deletes them then we'll get double-free crash delete runnables[runs-1]; - delete[] runnables; } void tst_QThreadPool::destroyingWaitsForTasksToFinish() -- cgit v1.2.3 From 410a14cc768ad084a4e474ffd8b61471405fce0f Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Fri, 10 Feb 2017 10:22:40 +0100 Subject: Wait for runnables to start up in tst_QThreadPool::cancel() In order to get reproducible runs of the test, we need to wait in the main thread until all runnables have started executing. Otherwise, what the cancel() loop below actually does will vary from run to run. Change-Id: Ib912b0943e7bbd55c9480ae6fd4011ba20ac457e Reviewed-by: Thiago Macieira --- tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) (limited to 'tests/auto/corelib/thread') diff --git a/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp b/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp index dede0be170..152aa4abdd 100644 --- a/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp +++ b/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp @@ -958,16 +958,19 @@ void tst_QThreadPool::clear() void tst_QThreadPool::cancel() { QSemaphore sem(0); + QSemaphore startedThreads(0); + class BlockingRunnable : public QRunnable { public: QSemaphore & sem; + QSemaphore &startedThreads; QAtomicInt &dtorCounter; QAtomicInt &runCounter; int dummy; - explicit BlockingRunnable(QSemaphore &s, QAtomicInt &c, QAtomicInt &r) - : sem(s), dtorCounter(c), runCounter(r){} + explicit BlockingRunnable(QSemaphore &s, QSemaphore &started, QAtomicInt &c, QAtomicInt &r) + : sem(s), startedThreads(started), dtorCounter(c), runCounter(r){} ~BlockingRunnable() { @@ -976,6 +979,7 @@ void tst_QThreadPool::cancel() void run() { + startedThreads.release(); runCounter.fetchAndAddRelaxed(1); sem.acquire(); count.ref(); @@ -995,11 +999,14 @@ void tst_QThreadPool::cancel() QAtomicInt dtorCounter = 0; QAtomicInt runCounter = 0; for (int i = 0; i < runs; i++) { - runnables[i] = new BlockingRunnable(sem, dtorCounter, runCounter); + 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 threadPool.cancel(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 < runs; i++) { threadPool.cancel(runnables[i]); } -- cgit v1.2.3 From b4689401a5ea142c9e2136b77d9193792874400a Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Fri, 10 Feb 2017 11:29:38 +0100 Subject: tst_QThreadPool: don't deadlock when a cancel() test fails We keep the runnables from finishing by having them block on a QSemaphore::acquire() call inside run(). If we fail a test that precedes the call to sem.release() further into the test, the early return will cause the thread pool to be destroyed, which will then attempt to wait for the runnables to finished, which, in turn wait for the semaphore to be released. -> dead lock Fix by introducing a RAII object to release the semaphore with a sufficiently large number to unblock all runnables. That number will in some situations be too large, but that does not matter. Change-Id: I1ec7e29b37bc36309e93e6e30708cc7db3c9579c Reviewed-by: Thiago Macieira --- .../corelib/thread/qthreadpool/tst_qthreadpool.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'tests/auto/corelib/thread') diff --git a/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp b/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp index 152aa4abdd..5b7242fddb 100644 --- a/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp +++ b/tests/auto/corelib/thread/qthreadpool/tst_qthreadpool.cpp @@ -960,6 +960,21 @@ void tst_QThreadPool::cancel() 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: @@ -995,6 +1010,11 @@ void tst_QThreadPool::cancel() 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; -- cgit v1.2.3