summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorThiago Macieira <thiago.macieira@intel.com>2017-11-09 15:49:26 -0800
committerJani Heikkinen <jani.heikkinen@qt.io>2017-11-14 11:07:30 +0000
commit4870282117b43242d9c2cd6fbde8175b2a907b08 (patch)
tree2a70cd866481b6d54dab9735b37c0177206f0de2
parentd583030f2ca29dacce1805ad75855353f678f9d5 (diff)
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>
-rw-r--r--src/corelib/thread/qsemaphore.cpp8
-rw-r--r--tests/auto/corelib/thread/qsemaphore/tst_qsemaphore.cpp63
2 files changed, 68 insertions, 3 deletions
diff --git a/src/corelib/thread/qsemaphore.cpp b/src/corelib/thread/qsemaphore.cpp
index ce0c1c91df..f63a08774c 100644
--- a/src/corelib/thread/qsemaphore.cpp
+++ b/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();
diff --git a/tests/auto/corelib/thread/qsemaphore/tst_qsemaphore.cpp b/tests/auto/corelib/thread/qsemaphore/tst_qsemaphore.cpp
index 2970b2e118..ce093f769c 100644
--- a/tests/auto/corelib/thread/qsemaphore/tst_qsemaphore.cpp
+++ b/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;