diff options
author | Marc Mutz <marc.mutz@kdab.com> | 2016-10-17 13:00:04 +0200 |
---|---|---|
committer | Marc Mutz <marc.mutz@kdab.com> | 2016-11-22 11:32:35 +0000 |
commit | d6c8fab8805f5085568065cdc8bfbfddfcfd8f2e (patch) | |
tree | 6e3364402c50dc52496a6ca0df003b4722175aa9 | |
parent | 34c2a1dcf05661562cd00fe6dbdf884d8917933d (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.cpp | 4 | ||||
-rw-r--r-- | src/corelib/thread/qmutex.h | 38 | ||||
-rw-r--r-- | tests/auto/corelib/thread/qmutex/tst_qmutex.cpp | 114 |
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 |