summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMarc Mutz <marc.mutz@kdab.com>2016-10-17 13:00:04 +0200
committerMarc Mutz <marc.mutz@kdab.com>2016-11-22 11:32:35 +0000
commitd6c8fab8805f5085568065cdc8bfbfddfcfd8f2e (patch)
tree6e3364402c50dc52496a6ca0df003b4722175aa9
parent34c2a1dcf05661562cd00fe6dbdf884d8917933d (diff)
QMutex: make sure we try_lock_for no shorter than the duration passed
By templating on the <chrono> types and unconditionally using duration_cast to coerce the duration into a milliseconds, we allowed code such as mutex.try_lock_for(10us) to compile, which is misleading, since it's actually a zero- timeout try_lock(). Feedback from the std-discussions mailing list is that the wait_for functions should wait for _at least_ the duration given, because that is the natural direction of variance (tasks becoming ready to run might not get a CPU immediately, causing delays), while an interface that documents to wait _no more_ than the given duration is promising something it cannot fulfill. Fix by converting the given duration to the smallest number of milliseconds not less than the original duration. If that is not representable in an int, use INT_MAX, emulating the effect of a spurious wakeup, which are allowed to happen if the function returns false in that case. In the above example, the try_lock_for call is now equivalent to mutex.tryLock(1); The tryLock() docs state that the actual waiting time does not exceed the given milliseconds, but fixing that is a separate issue. Change-Id: Id4cbbea0ecc6fd2f94bb5aef28a1658be3728e52 Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
-rw-r--r--src/corelib/thread/qmutex.cpp4
-rw-r--r--src/corelib/thread/qmutex.h38
-rw-r--r--tests/auto/corelib/thread/qmutex/tst_qmutex.cpp114
3 files changed, 146 insertions, 10 deletions
diff --git a/src/corelib/thread/qmutex.cpp b/src/corelib/thread/qmutex.cpp
index 6e0fa4eedb..0aee4aeda4 100644
--- a/src/corelib/thread/qmutex.cpp
+++ b/src/corelib/thread/qmutex.cpp
@@ -275,7 +275,7 @@ bool QMutex::tryLock(int timeout) QT_MUTEX_LOCK_NOEXCEPT
Attempts to lock the mutex. This function returns \c true if the lock
was obtained; otherwise it returns \c false. If another thread has
- locked the mutex, this function will wait for at most \a duration
+ locked the mutex, this function will wait for at least \a duration
for the mutex to become available.
Note: Passing a negative duration as the \a duration is equivalent to
@@ -299,7 +299,7 @@ bool QMutex::tryLock(int timeout) QT_MUTEX_LOCK_NOEXCEPT
Attempts to lock the mutex. This function returns \c true if the lock
was obtained; otherwise it returns \c false. If another thread has
- locked the mutex, this function will wait at most until \a timePoint
+ locked the mutex, this function will wait at least until \a timePoint
for the mutex to become available.
Note: Passing a \a timePoint which has already passed is equivalent
diff --git a/src/corelib/thread/qmutex.h b/src/corelib/thread/qmutex.h
index 3a0e22e3bd..056ebdeaa5 100644
--- a/src/corelib/thread/qmutex.h
+++ b/src/corelib/thread/qmutex.h
@@ -46,8 +46,11 @@
#if QT_HAS_INCLUDE(<chrono>)
# include <chrono>
+# include <limits>
#endif
+class tst_QMutex;
+
QT_BEGIN_NAMESPACE
@@ -135,14 +138,7 @@ public:
template <class Rep, class Period>
bool try_lock_for(std::chrono::duration<Rep, Period> duration)
{
- // N4606 § 30.4.1.3 [thread.timedmutex.requirements]/5 specifies that
- // a duration less than or equal to duration.zero() shall result in a
- // try_lock, unlike QMutex's tryLock with a negative duration which
- // results in a lock.
-
- if (duration <= duration.zero())
- return tryLock(0);
- return tryLock(std::chrono::duration_cast<std::chrono::milliseconds>(duration).count());
+ return tryLock(convertToMilliseconds(duration));
}
// TimedLockable concept
@@ -162,6 +158,32 @@ public:
private:
Q_DISABLE_COPY(QMutex)
friend class QMutexLocker;
+ friend class ::tst_QMutex;
+
+#if QT_HAS_INCLUDE(<chrono>)
+ template<class Rep, class Period>
+ static int convertToMilliseconds(std::chrono::duration<Rep, Period> duration)
+ {
+ // N4606 § 30.4.1.3.5 [thread.timedmutex.requirements] specifies that a
+ // duration less than or equal to duration.zero() shall result in a
+ // try_lock, unlike QMutex's tryLock with a negative duration which
+ // results in a lock.
+
+ if (duration <= duration.zero())
+ return 0;
+
+ // when converting from 'duration' to milliseconds, make sure that
+ // the result is not shorter than 'duration':
+ std::chrono::milliseconds wait = std::chrono::duration_cast<std::chrono::milliseconds>(duration);
+ if (wait < duration)
+ wait += std::chrono::milliseconds(1);
+ Q_ASSERT(wait >= duration);
+ const auto ms = wait.count();
+ const auto maxInt = (std::numeric_limits<int>::max)();
+
+ return ms < maxInt ? int(ms) : maxInt;
+ }
+#endif
};
class Q_CORE_EXPORT QMutexLocker
diff --git a/tests/auto/corelib/thread/qmutex/tst_qmutex.cpp b/tests/auto/corelib/thread/qmutex/tst_qmutex.cpp
index b24ecfcd43..bf778e9fd1 100644
--- a/tests/auto/corelib/thread/qmutex/tst_qmutex.cpp
+++ b/tests/auto/corelib/thread/qmutex/tst_qmutex.cpp
@@ -44,8 +44,19 @@
class tst_QMutex : public QObject
{
Q_OBJECT
+public:
+ enum class TimeUnit {
+ Nanoseconds,
+ Microseconds,
+ Milliseconds,
+ Seconds,
+ };
+ Q_ENUM(TimeUnit);
+
private slots:
void initTestCase();
+ void convertToMilliseconds_data();
+ void convertToMilliseconds();
void tryLock_non_recursive();
void try_lock_for_non_recursive();
void try_lock_until_non_recursive();
@@ -122,6 +133,109 @@ void tst_QMutex::initTestCase()
initializeSystemTimersResolution();
}
+void tst_QMutex::convertToMilliseconds_data()
+{
+ QTest::addColumn<TimeUnit>("unit");
+ QTest::addColumn<double>("doubleValue");
+ QTest::addColumn<qint64>("intValue");
+ QTest::addColumn<qint64>("expected");
+
+
+ auto add = [](TimeUnit unit, double d, long long i, qint64 expected) {
+ const QScopedArrayPointer<char> enumName(QTest::toString(unit));
+ QTest::newRow(qPrintable(QString::asprintf("%s:%f:%lld", enumName.data(), d, i)))
+ << unit << d << qint64(i) << expected;
+ };
+
+ auto forAllUnitsAdd = [=](double d, long long i, qint64 expected) {
+ for (auto unit : {TimeUnit::Nanoseconds, TimeUnit::Microseconds, TimeUnit::Milliseconds, TimeUnit::Seconds})
+ add(unit, d, i, expected);
+ };
+
+ forAllUnitsAdd(-0.5, -1, 0); // all negative values result in 0
+
+ forAllUnitsAdd(0, 0, 0);
+
+ add(TimeUnit::Nanoseconds, 1, 1, 1);
+ add(TimeUnit::Nanoseconds, 1000 * 1000, 1000 * 1000, 1);
+ add(TimeUnit::Nanoseconds, 1000 * 1000 + 0.5, 1000 * 1000 + 1, 2);
+
+ add(TimeUnit::Microseconds, 1, 1, 1);
+ add(TimeUnit::Microseconds, 1000, 1000, 1);
+ add(TimeUnit::Microseconds, 1000 + 0.5, 1000 + 1, 2);
+
+ add(TimeUnit::Milliseconds, 1, 1, 1);
+ add(TimeUnit::Milliseconds, 1.5, 2, 2);
+
+ add(TimeUnit::Seconds, 0.9991, 1, 1000);
+
+ //
+ // overflowing int results in INT_MAX (equivalent to a spurious wakeup after ~24 days); check it:
+ //
+
+ // spot on:
+ add(TimeUnit::Nanoseconds, INT_MAX * 1000. * 1000, INT_MAX * Q_INT64_C(1000) * 1000, INT_MAX);
+ add(TimeUnit::Microseconds, INT_MAX * 1000., INT_MAX * Q_INT64_C(1000), INT_MAX);
+ add(TimeUnit::Milliseconds, INT_MAX, INT_MAX, INT_MAX);
+
+ // minimally above:
+ add(TimeUnit::Nanoseconds, INT_MAX * 1000. * 1000 + 1, INT_MAX * Q_INT64_C(1000) * 1000 + 1, INT_MAX);
+ add(TimeUnit::Microseconds, INT_MAX * 1000. + 1, INT_MAX * Q_INT64_C(1000) + 1, INT_MAX);
+ add(TimeUnit::Milliseconds, INT_MAX + 1., INT_MAX + Q_INT64_C(1), INT_MAX);
+ add(TimeUnit::Seconds, INT_MAX / 1000. + 1, INT_MAX / 1000 + 1, INT_MAX);
+
+ // minimally below:
+ add(TimeUnit::Nanoseconds, INT_MAX * 1000. * 1000 - 1, INT_MAX * Q_INT64_C(1000) * 1000 - 1, INT_MAX);
+ add(TimeUnit::Microseconds, INT_MAX * 1000. - 1, INT_MAX * Q_INT64_C(1000) - 1, INT_MAX);
+ add(TimeUnit::Milliseconds, INT_MAX - 0.1, INT_MAX , INT_MAX);
+
+}
+
+void tst_QMutex::convertToMilliseconds()
+{
+#if !QT_HAS_INCLUDE(<chrono>)
+ QSKIP("This test requires <chrono>");
+#else
+ QFETCH(TimeUnit, unit);
+ QFETCH(double, doubleValue);
+ QFETCH(qint64, intValue);
+ QFETCH(qint64, expected);
+
+ Q_CONSTEXPR qint64 maxShort = std::numeric_limits<short>::max();
+ Q_CONSTEXPR qint64 maxInt = std::numeric_limits<int>::max();
+ Q_CONSTEXPR qint64 maxUInt = std::numeric_limits<uint>::max();
+
+ switch (unit) {
+#define CASE(Unit, Period) \
+ case TimeUnit::Unit: \
+ DO(double, Period, doubleValue); \
+ if (intValue < maxShort) \
+ DO(short, Period, short(intValue)); \
+ if (intValue < maxInt) \
+ DO(int, Period, int(intValue)); \
+ DO(qint64, Period, intValue); \
+ if (intValue >= 0) { \
+ if (intValue < maxUInt) \
+ DO(uint, Period, uint(intValue)); \
+ DO(quint64, Period, quint64(intValue)); \
+ } \
+ break
+#define DO(Rep, Period, val) \
+ do { \
+ const std::chrono::duration<Rep, Period> wait((val)); \
+ QCOMPARE(QMutex::convertToMilliseconds(wait), expected); \
+ } while (0)
+
+ CASE(Nanoseconds, std::nano);
+ CASE(Microseconds, std::micro);
+ CASE(Milliseconds, std::milli);
+ CASE(Seconds, std::ratio<1>);
+#undef DO
+#undef CASE
+ }
+#endif
+}
+
void tst_QMutex::tryLock_non_recursive()
{
class Thread : public QThread