diff --git a/ci/build-travis.sh b/ci/build-travis.sh index 1478d18d7..ccb952e5d 100755 --- a/ci/build-travis.sh +++ b/ci/build-travis.sh @@ -14,7 +14,7 @@ if [[ $(grep -rl --exclude="*asio.hpp" "asio::async_write" ./nano) ]]; then exit 1 fi -# prevent unsolicited use of std::lock_guard & std::unique_lock outside of allowed areas +# prevent unsolicited use of std::lock_guard, std::unique_lock & std::condition_variable outside of allowed areas if [[ $(grep -rl --exclude={"*random_pool.cpp","*random_pool.hpp","*random_pool_shuffle.hpp","*locks.hpp","*locks.cpp"} "std::unique_lock\|std::lock_guard\|std::condition_variable" ./nano) ]]; then echo "Using std::unique_lock, std::lock_guard or std::condition_variable is not permitted (except in nano/lib/locks.hpp and non-nano dependent libraries). Use the nano::* versions instead" exit 1 diff --git a/nano/core_test/locks.cpp b/nano/core_test/locks.cpp index cfcd51f12..f0a8994ef 100644 --- a/nano/core_test/locks.cpp +++ b/nano/core_test/locks.cpp @@ -92,7 +92,7 @@ TEST (locks, unique_lock) lk.lock (); promise.set_value (); - // Tries to make sure that the other guard to held for a minimum of NANO_TIMED_LOCKS, may need to increase this for low NANO_TIMED_LOCKS values + // Tries to make sure that the other guard is held for a minimum of NANO_TIMED_LOCKS, may need to increase this for low NANO_TIMED_LOCKS values std::this_thread::sleep_for (std::chrono::milliseconds (NANO_TIMED_LOCKS * 2)); }); @@ -107,14 +107,20 @@ TEST (locks, unique_lock) ASSERT_EQ (num_matches (ss.str ()), 4); } -TEST (locks, condition_variable) +TEST (locks, condition_variable_wait) { + // This test can end up taking a long time, as it sleeps for the NANO_TIMED_LOCKS amount + ASSERT_LE (NANO_TIMED_LOCKS, 10000); + + std::stringstream ss; + nano::cout_redirect redirect (ss.rdbuf ()); + nano::condition_variable cv; std::mutex mutex; - std::promise promise; - std::atomic finished{ false }; std::atomic notified{ false }; - std::thread t ([&cv, ¬ified, &finished] { + std::atomic finished{ false }; + std::thread t ([&] { + std::this_thread::sleep_for (std::chrono::milliseconds (NANO_TIMED_LOCKS * 2)); while (!finished) { notified = true; @@ -123,11 +129,53 @@ TEST (locks, condition_variable) }); nano::unique_lock lk (mutex); + std::this_thread::sleep_for (std::chrono::milliseconds (NANO_TIMED_LOCKS)); cv.wait (lk, [¬ified] { return notified.load (); }); - finished = true; + t.join (); + // 1 mutex held + ASSERT_EQ (num_matches (ss.str ()), 1); +} + +TEST (locks, condition_variable_wait_until) +{ + // This test can end up taking a long time, as it sleeps for the NANO_TIMED_LOCKS amount + ASSERT_LE (NANO_TIMED_LOCKS, 10000); + + std::stringstream ss; + nano::cout_redirect redirect (ss.rdbuf ()); + + nano::condition_variable cv; + std::mutex mutex; + auto impl = [&](auto time_to_sleep) { + std::atomic notified{ false }; + std::atomic finished{ false }; + nano::unique_lock lk (mutex); + std::this_thread::sleep_for (std::chrono::milliseconds (time_to_sleep)); + std::thread t ([&] { + while (!finished) + { + notified = true; + cv.notify_one (); + } + }); + + cv.wait_until (lk, std::chrono::steady_clock::now () + std::chrono::milliseconds (NANO_TIMED_LOCKS), [¬ified] { + return notified.load (); + }); + finished = true; + lk.unlock (); + t.join (); + }; + + impl (0); + // wait_until should not report any stacktraces + ASSERT_EQ (num_matches (ss.str ()), 0); + impl (NANO_TIMED_LOCKS); + // Should be 1 report + ASSERT_EQ (num_matches (ss.str ()), 1); } #endif diff --git a/nano/lib/locks.cpp b/nano/lib/locks.cpp index 8c784379d..931df118b 100644 --- a/nano/lib/locks.cpp +++ b/nano/lib/locks.cpp @@ -1,10 +1,10 @@ +#if NANO_TIMED_LOCKS > 0 #include #include #include -#if NANO_TIMED_LOCKS > 0 -namespace +namespace nano { template void output (const char * str, std::chrono::milliseconds time, Mutex & mutex) @@ -25,7 +25,10 @@ void output_if_held_long_enough (nano::timer & timer, { output ("held", time_held, mutex); } - timer.stop (); + if (timer.current_state () != nano::timer_state::stopped) + { + timer.stop (); + } } #ifndef NANO_TIMED_LOCKS_IGNORE_BLOCKED @@ -39,10 +42,12 @@ void output_if_blocked_long_enough (nano::timer & tim } } #endif -} -namespace nano -{ +// Explicit instantations +template void output (const char * str, std::chrono::milliseconds time, std::mutex & mutex); +template void output_if_held_long_enough (nano::timer & timer, std::mutex & mutex); +template void output_if_blocked_long_enough (nano::timer & timer, std::mutex & mutex); + lock_guard::lock_guard (std::mutex & mutex) : mut (mutex) { @@ -60,9 +65,6 @@ lock_guard::~lock_guard () noexcept output_if_held_long_enough (timer, mut); } -// Explicit instantiations for allowed types -template class lock_guard; - template unique_lock::unique_lock (Mutex & mutex) : mut (std::addressof (mutex)) @@ -186,5 +188,29 @@ void unique_lock::validate () const // Explicit instantiations for allowed types template class unique_lock; + +void condition_variable::notify_one () noexcept +{ + cnd.notify_one (); +} + +void condition_variable::notify_all () noexcept +{ + cnd.notify_all (); +} + +void condition_variable::wait (nano::unique_lock & lk) +{ + if (!lk.mut || !lk.owns) + { + throw (std::system_error (std::make_error_code (std::errc::operation_not_permitted))); + } + + output_if_held_long_enough (lk.timer, *lk.mut); + // Start again in case cnd.wait calls unique_lock::lock/unlock () depending on some implementations + lk.timer.start (); + cnd.wait (lk); + lk.timer.restart (); +} } #endif diff --git a/nano/lib/locks.hpp b/nano/lib/locks.hpp index f0b34bdb6..aaf529329 100644 --- a/nano/lib/locks.hpp +++ b/nano/lib/locks.hpp @@ -1,14 +1,24 @@ #pragma once +#if NANO_TIMED_LOCKS > 0 #include +#endif #include #include -#include namespace nano { #if NANO_TIMED_LOCKS > 0 +template +void output (const char * str, std::chrono::milliseconds time, Mutex & mutex); + +template +void output_if_held_long_enough (nano::timer & timer, Mutex & mutex); + +template +void output_if_blocked_long_enough (nano::timer & timer, Mutex & mutex); + template class lock_guard final { @@ -67,6 +77,76 @@ private: void validate () const; void lock_impl (); + + friend class condition_variable; +}; + +/** Assumes std implementations of std::condition_variable never actually call nano::unique_lock::lock/unlock, + but instead use OS intrinsics with the mutex handle directly. Due to this we also do not account for any + time the condition variable is blocked on another holder of the mutex. */ +class condition_variable final +{ +public: + condition_variable () = default; + condition_variable (condition_variable const &) = delete; + condition_variable & operator= (condition_variable const &) = delete; + + void notify_one () noexcept; + void notify_all () noexcept; + void wait (nano::unique_lock & lt); + + template + void wait (nano::unique_lock & lk, Pred pred) + { + while (!pred ()) + { + wait (lk); + } + } + + template + std::cv_status wait_until (nano::unique_lock & lk, std::chrono::time_point const & timeout_time) + { + if (!lk.mut || !lk.owns) + { + throw (std::system_error (std::make_error_code (std::errc::operation_not_permitted))); + } + + output_if_held_long_enough (lk.timer, *lk.mut); + // Start again in case cnd.wait calls unique_lock::lock/unlock () depending on some implementations + lk.timer.start (); + auto cv_status = cnd.wait_until (lk, timeout_time); + lk.timer.restart (); + return cv_status; + } + + template + bool wait_until (nano::unique_lock & lk, std::chrono::time_point const & timeout_time, Pred pred) + { + while (!pred ()) + { + if (wait_until (lk, timeout_time) == std::cv_status::timeout) + { + return pred (); + } + } + return true; + } + + template + void wait_for (nano::unique_lock & lk, std::chrono::duration const & rel_time) + { + wait_until (lk, std::chrono::steady_clock::now () + rel_time); + } + + template + bool wait_for (nano::unique_lock & lk, std::chrono::duration const & rel_time, Pred pred) + { + return wait_until (lk, std::chrono::steady_clock::now () + rel_time, std::move (pred)); + } + +private: + std::condition_variable_any cnd; }; #else @@ -75,10 +155,10 @@ using lock_guard = std::lock_guard; template using unique_lock = std::unique_lock; -#endif // For consistency wrapping the less well known _any variant which can be used with any lockable type using condition_variable = std::condition_variable_any; +#endif /** A general purpose monitor template */ template diff --git a/nano/node/confirmation_height_bounded.hpp b/nano/node/confirmation_height_bounded.hpp index 586ea464b..14599fcec 100644 --- a/nano/node/confirmation_height_bounded.hpp +++ b/nano/node/confirmation_height_bounded.hpp @@ -2,6 +2,7 @@ #include #include +#include #include #include diff --git a/nano/node/confirmation_height_processor.hpp b/nano/node/confirmation_height_processor.hpp index a0cfb6ecb..0146cc326 100644 --- a/nano/node/confirmation_height_processor.hpp +++ b/nano/node/confirmation_height_processor.hpp @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include #include diff --git a/nano/node/confirmation_height_unbounded.hpp b/nano/node/confirmation_height_unbounded.hpp index 20cfc5441..057a92263 100644 --- a/nano/node/confirmation_height_unbounded.hpp +++ b/nano/node/confirmation_height_unbounded.hpp @@ -2,6 +2,7 @@ #include #include +#include #include #include diff --git a/nano/node/state_block_signature_verification.cpp b/nano/node/state_block_signature_verification.cpp index 9dc670022..e5eb667a2 100644 --- a/nano/node/state_block_signature_verification.cpp +++ b/nano/node/state_block_signature_verification.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include