1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QMutex: make sure we try_lock_for no shorter than the duration passed

Browse Source 
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>
This commit is contained in:
Marc Mutz 2016-10-17 13:00:04 +02:00
parent 34c2a1dcf0
commit d6c8fab880
3 changed files with 146 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/corelib/thread/qmutex.cpp
View File

@ -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

38
src/corelib/thread/qmutex.h
View File

@ -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

114
tests/auto/corelib/thread/qmutex/tst_qmutex.cpp
View File

@ -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