QSemaphore::tryAquire(timeout) -- never times out on an active semaphore

If a thread trying to acquire multiple resources is continuously
preempted by threads acquiring smaller amounts, the larger consumer
would end up waiting forever (instead of for the given timeout).

Fix this by keeping track of elapsed time between wakeups using
QElapsedTimer.

Task-number: QTBUG-11500
Reviewed-by: thiago

2 files changed, 53 insertions, 3 deletions

diff --git a/src/corelib/thread/qsemaphore.cpp b/src/corelib/thread/qsemaphore.cpp
index 9dc828d..8e8a88a 100644
--- a/src/corelib/thread/qsemaphore.cpp
+++ b/src/corelib/thread/qsemaphore.cpp
@@ -44,6 +44,8 @@
 #ifndef QT_NO_THREAD
 #include "qmutex.h"
 #include "qwaitcondition.h"
+#include "qelapsedtimer.h"
+#include "qdatetime.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -218,8 +220,11 @@ bool QSemaphore::tryAcquire(int n, int timeout)
         while (n > d->avail)
             d->cond.wait(locker.mutex());
     } else {
+        QElapsedTimer timer;
+        timer.start();
         while (n > d->avail) {
-            if (!d->cond.wait(locker.mutex(), timeout))
+            if (timer.hasExpired(timeout)
+                || !d->cond.wait(locker.mutex(), timeout - timer.elapsed()))
                 return false;
         }
     }
diff --git a/tests/auto/qsemaphore/tst_qsemaphore.cpp b/tests/auto/qsemaphore/tst_qsemaphore.cpp
index ace33dc..7cede30 100644
--- a/tests/auto/qsemaphore/tst_qsemaphore.cpp
+++ b/tests/auto/qsemaphore/tst_qsemaphore.cpp
@@ -63,6 +63,7 @@ private slots:
     void tryAcquire();
     void tryAcquireWithTimeout_data();
     void tryAcquireWithTimeout();
+    void tryAcquireWithTimeoutStarvation();
     void release();
     void available();
     void producerConsumer();
@@ -232,8 +233,8 @@ void tst_QSemaphore::tryAcquireWithTimeout_data()
 {
     QTest::addColumn<int>("timeout");
 
-    QTest::newRow("") << 1000;
-    QTest::newRow("") << 10000;
+    QTest::newRow("1s") << 1000;
+    QTest::newRow("10s") << 10000;
 }
 
 void tst_QSemaphore::tryAcquireWithTimeout()
@@ -316,6 +317,50 @@ void tst_QSemaphore::tryAcquireWithTimeout()
     QCOMPARE(semaphore.available(), 0);
 }
 
+void tst_QSemaphore::tryAcquireWithTimeoutStarvation()
+{
+    class Thread : public QThread
+    {
+    public:
+        QSemaphore startup;
+        QSemaphore *semaphore;
+        int amountToConsume, timeout;
+
+        void run()
+        {
+            startup.release();
+            forever {
+                if (!semaphore->tryAcquire(amountToConsume, timeout))
+                    break;
+                semaphore->release(amountToConsume);
+            }
+        }
+    };
+
+    QSemaphore semaphore;
+    semaphore.release(1);
+
+    Thread consumer;
+    consumer.semaphore = &semaphore;
+    consumer.amountToConsume = 1;
+    consumer.timeout = 1000;
+
+    // start the thread and wait for it to start consuming
+    consumer.start();
+    consumer.startup.acquire();
+
+    // try to consume more than the thread we started is, and provide a longer
+    // timeout... we should timeout, not wait indefinitely
+    QVERIFY(!semaphore.tryAcquire(consumer.amountToConsume * 2, consumer.timeout * 2));
+
+    // the consumer should still be running
+    QVERIFY(consumer.isRunning() && !consumer.isFinished());
+
+    // acquire, and wait for smallConsumer to timeout
+    semaphore.acquire();
+    QVERIFY(consumer.wait());
+}
+
 void tst_QSemaphore::release()
 { DEPENDS_ON("acquire"); }