From d6c8fab8805f5085568065cdc8bfbfddfcfd8f2e Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Mon, 17 Oct 2016 13:00:04 +0200 Subject: QMutex: make sure we try_lock_for no shorter than the duration passed By templating on the 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 --- src/corelib/thread/qmutex.cpp | 4 +- src/corelib/thread/qmutex.h | 38 ++++++-- 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() # include +# include #endif +class tst_QMutex; + QT_BEGIN_NAMESPACE @@ -135,14 +138,7 @@ public: template bool try_lock_for(std::chrono::duration 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(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() + template + static int convertToMilliseconds(std::chrono::duration 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(duration); + if (wait < duration) + wait += std::chrono::milliseconds(1); + Q_ASSERT(wait >= duration); + const auto ms = wait.count(); + const auto maxInt = (std::numeric_limits::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("unit"); + QTest::addColumn("doubleValue"); + QTest::addColumn("intValue"); + QTest::addColumn("expected"); + + + auto add = [](TimeUnit unit, double d, long long i, qint64 expected) { + const QScopedArrayPointer 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() + QSKIP("This test requires "); +#else + QFETCH(TimeUnit, unit); + QFETCH(double, doubleValue); + QFETCH(qint64, intValue); + QFETCH(qint64, expected); + + Q_CONSTEXPR qint64 maxShort = std::numeric_limits::max(); + Q_CONSTEXPR qint64 maxInt = std::numeric_limits::max(); + Q_CONSTEXPR qint64 maxUInt = std::numeric_limits::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 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 -- cgit v1.2.3