From 97ffc69bea04758e1c7f94e1075ef2bcce652999 Mon Sep 17 00:00:00 2001 From: Ben Caimano Date: Wed, 9 Oct 2019 21:40:44 +0000 Subject: SERVER-43800 ClockSource::waitForConditionUntil shouldn't use unique_lock out of line --- src/mongo/transport/baton_asio_linux.h | 3 +- src/mongo/util/clock_source.cpp | 66 +++++++++++++++++----------------- src/mongo/util/clock_source.h | 6 ++++ src/mongo/util/waitable.h | 3 ++ 4 files changed, 43 insertions(+), 35 deletions(-) diff --git a/src/mongo/transport/baton_asio_linux.h b/src/mongo/transport/baton_asio_linux.h index dd5062cab72..930cf52c67d 100644 --- a/src/mongo/transport/baton_asio_linux.h +++ b/src/mongo/transport/baton_asio_linux.h @@ -305,7 +305,8 @@ public: // If we don't have a timeout, or we have a timeout that's unexpired, run poll. if (!deadline || (*deadline > now)) { if (deadline && !clkSource->tracksSystemClock()) { - invariant(clkSource->setAlarm(*deadline, [this] { notify(); })); + invariant(clkSource->setAlarm(*deadline, + [this, anchor = shared_from_this()] { notify(); })); deadline.reset(); } diff --git a/src/mongo/util/clock_source.cpp b/src/mongo/util/clock_source.cpp index ebae9737e73..cd3da75e9c9 100644 --- a/src/mongo/util/clock_source.cpp +++ b/src/mongo/util/clock_source.cpp @@ -35,61 +35,59 @@ namespace mongo { stdx::cv_status ClockSource::waitForConditionUntil(stdx::condition_variable& cv, - BasicLockableAdapter m, + BasicLockableAdapter bla, Date_t deadline, Waitable* waitable) { if (_tracksSystemClock) { if (deadline == Date_t::max()) { - Waitable::wait(waitable, this, cv, m); + Waitable::wait(waitable, this, cv, bla); return stdx::cv_status::no_timeout; } - return Waitable::wait_until(waitable, this, cv, m, deadline.toSystemTimePoint()); + return Waitable::wait_until(waitable, this, cv, bla, deadline.toSystemTimePoint()); } // The rest of this function only runs during testing, when the clock source is virtualized and // does not track the system clock. - if (deadline <= now()) { + auto currentTime = now(); + if (deadline <= currentTime) { return stdx::cv_status::timeout; } struct AlarmInfo { - Mutex controlMutex = MONGO_MAKE_LATCH("AlarmInfo::controlMutex"); - boost::optional waitLock; - stdx::condition_variable* waitCV; - stdx::cv_status cvWaitResult = stdx::cv_status::no_timeout; + stdx::mutex mutex; // NOLINT + + stdx::condition_variable* cv; + stdx::cv_status result = stdx::cv_status::no_timeout; }; auto alarmInfo = std::make_shared(); - alarmInfo->waitCV = &cv; - alarmInfo->waitLock = m; - const auto waiterThreadId = stdx::this_thread::get_id(); - bool invokedAlarmInline = false; - invariant(setAlarm(deadline, [alarmInfo, waiterThreadId, &invokedAlarmInline] { - stdx::lock_guard controlLk(alarmInfo->controlMutex); - alarmInfo->cvWaitResult = stdx::cv_status::timeout; - if (!alarmInfo->waitLock) { - return; - } - if (stdx::this_thread::get_id() == waiterThreadId) { - // In NetworkInterfaceMock, setAlarm may invoke its callback immediately if the deadline - // has expired, so we detect that case and avoid self-deadlock by returning early, here. - // It is safe to set invokedAlarmInline without synchronization in this case, because it - // is exactly the case where the same thread is writing and consulting the value. - invokedAlarmInline = true; + alarmInfo->cv = &cv; + + invariant(setAlarm(deadline, [alarmInfo] { + // Set an alarm to hit our virtualized deadline + stdx::lock_guard infoLk(alarmInfo->mutex); + auto cv = std::exchange(alarmInfo->cv, nullptr); + if (!cv) { return; } - stdx::lock_guard waitLk(*alarmInfo->waitLock); - alarmInfo->waitCV->notify_all(); + + alarmInfo->result = stdx::cv_status::timeout; + cv->notify_all(); })); - if (!invokedAlarmInline) { - Waitable::wait(waitable, this, cv, m); + + if (stdx::lock_guard infoLk(alarmInfo->mutex); !alarmInfo->cv) { + // If setAlarm() ran inline, then we've timed out + return alarmInfo->result; } - m.unlock(); - stdx::lock_guard controlLk(alarmInfo->controlMutex); - m.lock(); - alarmInfo->waitLock = boost::none; - alarmInfo->waitCV = nullptr; - return alarmInfo->cvWaitResult; + + // This is a wait_until because theoretically setAlarm could run out of line before this cv + // joins the wait list. Then it could completely miss the notification and block until a lucky + // renotify or spurious wakeup. + Waitable::wait_until(waitable, this, cv, bla, currentTime + kMaxTimeoutForArtificialClocks); + + stdx::lock_guard infoLk(alarmInfo->mutex); + alarmInfo->cv = nullptr; + return alarmInfo->result; } } // namespace mongo diff --git a/src/mongo/util/clock_source.h b/src/mongo/util/clock_source.h index c27d22b1dd3..f202f67f439 100644 --- a/src/mongo/util/clock_source.h +++ b/src/mongo/util/clock_source.h @@ -55,6 +55,8 @@ class ClockSource { std::is_function>::value> { }; + static constexpr auto kMaxTimeoutForArtificialClocks = Seconds(1); + public: virtual ~ClockSource() = default; @@ -90,6 +92,10 @@ public: /** * Like cv.wait_until(m, deadline), but uses this ClockSource instead of * stdx::chrono::system_clock to measure the passage of time. + * + * Note that this can suffer spurious wakeups like cw.wait_until() and, when used with a mocked + * clock source, may sleep in system time for kMaxTimeoutForArtificialClocks due to unfortunate + * implementation details. */ stdx::cv_status waitForConditionUntil(stdx::condition_variable& cv, BasicLockableAdapter m, diff --git a/src/mongo/util/waitable.h b/src/mongo/util/waitable.h index 482c7979dbf..b831d422b23 100644 --- a/src/mongo/util/waitable.h +++ b/src/mongo/util/waitable.h @@ -46,6 +46,9 @@ namespace mongo { * * The current implementer of Waitable is the transport layer baton type, which performs delayed IO * when it would otherwise block. + * + * Note that every Waitable should be level-triggered like its base class, Notifyable. See + * mongo/stdx/condition_variable.h for more details. */ class Waitable : public Notifyable { public: -- cgit v1.2.1