QSemaphore: fix regression when the timeout < 0

The issue was introduced by eaee1209f0ead5be786e81db8aee604ccfea85b0, so
it affected only 5.9.2.

[ChangeLog][QtCore][QSemaphore] Fixed a regression that would make
tryAcquire() not to wait forever if the timeout was a negative
value. Note: new code is advised to only use -1 to indicate "forever",
as some other functions taking timeout periods do not accept other
values.

Task-number: QTBUG-64413
Change-Id: I57a1bd6e0c194530b732fffd14f58fce60d5dfc9
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

src/corelib/thread/qsemaphore.cpp

@@ -214,13 +214,15 @@ bool QSemaphore::tryAcquire(int n)
 bool QSemaphore::tryAcquire(int n, int timeout)
 {
     Q_ASSERT_X(n >= 0, "QSemaphore::tryAcquire", "parameter 'n' must be non-negative");
-    if (timeout < 0)
+
-        return tryAcquire(n);
+    // We're documented to accept any negative value as "forever"
+    // but QDeadlineTimer only accepts -1.
+    timeout = qMax(timeout, -1);
 
     QDeadlineTimer timer(timeout);
     QMutexLocker locker(&d->mutex);
     qint64 remainingTime = timer.remainingTime();
-    while (n > d->avail && remainingTime > 0) {
+    while (n > d->avail && remainingTime != 0) {
         if (!d->cond.wait(locker.mutex(), remainingTime))
             return false;
         remainingTime = timer.remainingTime();

tests/auto/corelib/thread/qsemaphore/tst_qsemaphore.cpp

@@ -41,6 +41,8 @@ private slots:
     void tryAcquireWithTimeout_data();
     void tryAcquireWithTimeout();
     void tryAcquireWithTimeoutStarvation();
+    void tryAcquireWithTimeoutForever_data();
+    void tryAcquireWithTimeoutForever();
     void producerConsumer();
 };
 
@@ -155,21 +157,25 @@ void tst_QSemaphore::tryAcquire()
     semaphore.release();
     QCOMPARE(semaphore.available(), 1);
     QVERIFY(!semaphore.tryAcquire(2));
+    QVERIFY(!semaphore.tryAcquire(2, 0));
     QCOMPARE(semaphore.available(), 1);
 
     semaphore.release();
     QCOMPARE(semaphore.available(), 2);
     QVERIFY(!semaphore.tryAcquire(3));
+    QVERIFY(!semaphore.tryAcquire(3, 0));
     QCOMPARE(semaphore.available(), 2);
 
     semaphore.release(10);
     QCOMPARE(semaphore.available(), 12);
     QVERIFY(!semaphore.tryAcquire(100));
+    QVERIFY(!semaphore.tryAcquire(100, 0));
     QCOMPARE(semaphore.available(), 12);
 
     semaphore.release(10);
     QCOMPARE(semaphore.available(), 22);
     QVERIFY(!semaphore.tryAcquire(100));
+    QVERIFY(!semaphore.tryAcquire(100, 0));
     QCOMPARE(semaphore.available(), 22);
 
     QVERIFY(semaphore.tryAcquire());
@@ -178,23 +184,38 @@ void tst_QSemaphore::tryAcquire()
     QVERIFY(semaphore.tryAcquire());
     QCOMPARE(semaphore.available(), 20);
 
+    semaphore.release(2);
+    QVERIFY(semaphore.tryAcquire(1, 0));
+    QCOMPARE(semaphore.available(), 21);
+
+    QVERIFY(semaphore.tryAcquire(1, 0));
+    QCOMPARE(semaphore.available(), 20);
+
     QVERIFY(semaphore.tryAcquire(10));
     QCOMPARE(semaphore.available(), 10);
 
+    semaphore.release(10);
+    QVERIFY(semaphore.tryAcquire(10, 0));
+    QCOMPARE(semaphore.available(), 10);
+
     QVERIFY(semaphore.tryAcquire(10));
     QCOMPARE(semaphore.available(), 0);
 
     // should not be able to acquire more
     QVERIFY(!semaphore.tryAcquire());
+    QVERIFY(!semaphore.tryAcquire(1, 0));
     QCOMPARE(semaphore.available(), 0);
 
     QVERIFY(!semaphore.tryAcquire());
+    QVERIFY(!semaphore.tryAcquire(1, 0));
     QCOMPARE(semaphore.available(), 0);
 
     QVERIFY(!semaphore.tryAcquire(10));
+    QVERIFY(!semaphore.tryAcquire(10, 0));
     QCOMPARE(semaphore.available(), 0);
 
     QVERIFY(!semaphore.tryAcquire(10));
+    QVERIFY(!semaphore.tryAcquire(10, 0));
     QCOMPARE(semaphore.available(), 0);
 }
 
@@ -344,6 +365,48 @@ void tst_QSemaphore::tryAcquireWithTimeoutStarvation()
     QVERIFY(consumer.wait());
 }
 
+void tst_QSemaphore::tryAcquireWithTimeoutForever_data()
+{
+    QTest::addColumn<int>("timeout");
+    QTest::newRow("-1") << -1;
+
+    // tryAcquire is documented to take any negative value as "forever"
+    QTest::newRow("INT_MIN") << INT_MIN;
+}
+
+void tst_QSemaphore::tryAcquireWithTimeoutForever()
+{
+    enum { WaitTime = 1000 };
+    struct Thread : public QThread {
+        QSemaphore sem;
+
+        void run() override
+        {
+            QTest::qWait(WaitTime);
+            sem.release(2);
+        }
+    };
+
+    QFETCH(int, timeout);
+    Thread t;
+
+    // sanity check it works if we can immediately acquire
+    t.sem.release(11);
+    QVERIFY(t.sem.tryAcquire(1, timeout));
+    QVERIFY(t.sem.tryAcquire(10, timeout));
+
+    // verify that we do wait for at least WaitTime if we can't acquire immediately
+    QElapsedTimer timer;
+    timer.start();
+    t.start();
+    QVERIFY(t.sem.tryAcquire(1, timeout));
+    QVERIFY(timer.elapsed() >= WaitTime);
+
+    QVERIFY(t.wait());
+
+    QCOMPARE(t.sem.available(), 1);
+}
+
 const char alphabet[] = "ACGTH";
 const int AlphabetSize = sizeof(alphabet) - 1;