Make std::condition_variable::wait* compatible with NANO_TIMED_LOCKS (#2679)

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

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<void> promise;
-	std::atomic<bool> finished{ false };
 	std::atomic<bool> notified{ false };
-	std::thread t ([&cv, &notified, &finished] {
+	std::atomic<bool> 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<std::mutex> lk (mutex);
+	std::this_thread::sleep_for (std::chrono::milliseconds (NANO_TIMED_LOCKS));
 	cv.wait (lk, [&notified] {
 		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<bool> notified{ false };
+		std::atomic<bool> finished{ false };
+		nano::unique_lock<std::mutex> 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), [&notified] {
+			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

nano/lib/locks.cpp

@@ -1,10 +1,10 @@
+#if NANO_TIMED_LOCKS > 0
 #include <nano/lib/locks.hpp>
 #include <nano/lib/utility.hpp>
 
 #include <iostream>
 
-#if NANO_TIMED_LOCKS > 0
+namespace nano
-namespace
 {
 template <typename Mutex>
 void output (const char * str, std::chrono::milliseconds time, Mutex & mutex)
@@ -25,7 +25,10 @@ void output_if_held_long_enough (nano::timer<std::chrono::milliseconds> & 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<std::chrono::milliseconds> & 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<std::chrono::milliseconds> & timer, std::mutex & mutex);
+template void output_if_blocked_long_enough (nano::timer<std::chrono::milliseconds> & timer, std::mutex & mutex);
+
 lock_guard<std::mutex>::lock_guard (std::mutex & mutex) :
 mut (mutex)
 {
@@ -60,9 +65,6 @@ lock_guard<std::mutex>::~lock_guard () noexcept
 	output_if_held_long_enough (timer, mut);
 }
 
-// Explicit instantiations for allowed types
-template class lock_guard<std::mutex>;
-
 template <typename Mutex, typename U>
 unique_lock<Mutex, U>::unique_lock (Mutex & mutex) :
 mut (std::addressof (mutex))
@@ -186,5 +188,29 @@ void unique_lock<Mutex, U>::validate () const
 
 // Explicit instantiations for allowed types
 template class unique_lock<std::mutex>;
+
+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<std::mutex> & 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

nano/lib/locks.hpp

@@ -1,14 +1,24 @@
 #pragma once
 
+#if NANO_TIMED_LOCKS > 0
 #include <nano/lib/timer.hpp>
+#endif
 
 #include <condition_variable>
 #include <mutex>
-#include <unordered_map>
 
 namespace nano
 {
 #if NANO_TIMED_LOCKS > 0
+template <typename Mutex>
+void output (const char * str, std::chrono::milliseconds time, Mutex & mutex);
+
+template <typename Mutex>
+void output_if_held_long_enough (nano::timer<std::chrono::milliseconds> & timer, Mutex & mutex);
+
+template <typename Mutex>
+void output_if_blocked_long_enough (nano::timer<std::chrono::milliseconds> & timer, Mutex & mutex);
+
 template <typename Mutex>
 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<std::mutex> & lt);
+
+	template <typename Pred>
+	void wait (nano::unique_lock<std::mutex> & lk, Pred pred)
+	{
+		while (!pred ())
+		{
+			wait (lk);
+		}
+	}
+
+	template <typename Clock, typename Duration>
+	std::cv_status wait_until (nano::unique_lock<std::mutex> & lk, std::chrono::time_point<Clock, Duration> 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 <typename Clock, typename Duration, typename Pred>
+	bool wait_until (nano::unique_lock<std::mutex> & lk, std::chrono::time_point<Clock, Duration> const & timeout_time, Pred pred)
+	{
+		while (!pred ())
+		{
+			if (wait_until (lk, timeout_time) == std::cv_status::timeout)
+			{
+				return pred ();
+			}
+		}
+		return true;
+	}
+
+	template <typename Rep, typename Period>
+	void wait_for (nano::unique_lock<std::mutex> & lk, std::chrono::duration<Rep, Period> const & rel_time)
+	{
+		wait_until (lk, std::chrono::steady_clock::now () + rel_time);
+	}
+
+	template <typename Rep, typename Period, typename Pred>
+	bool wait_for (nano::unique_lock<std::mutex> & lk, std::chrono::duration<Rep, Period> 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<Mutex>;
 
 template <typename Mutex>
 using unique_lock = std::unique_lock<Mutex>;
-#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 <class T>

nano/node/confirmation_height_bounded.hpp

@@ -2,6 +2,7 @@
 
 #include <nano/lib/numbers.hpp>
 #include <nano/lib/threading.hpp>
+#include <nano/lib/timer.hpp>
 #include <nano/secure/blockstore.hpp>
 
 #include <boost/circular_buffer.hpp>

nano/node/confirmation_height_processor.hpp

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <nano/lib/numbers.hpp>
+#include <nano/lib/timer.hpp>
 #include <nano/node/confirmation_height_bounded.hpp>
 #include <nano/node/confirmation_height_unbounded.hpp>
 #include <nano/secure/blockstore.hpp>

nano/node/confirmation_height_unbounded.hpp

@@ -2,6 +2,7 @@
 
 #include <nano/lib/numbers.hpp>
 #include <nano/lib/threading.hpp>
+#include <nano/lib/timer.hpp>
 #include <nano/secure/blockstore.hpp>
 
 #include <chrono>

nano/node/state_block_signature_verification.cpp

@@ -1,6 +1,7 @@
 #include <nano/lib/logger_mt.hpp>
 #include <nano/lib/numbers.hpp>
 #include <nano/lib/threading.hpp>
+#include <nano/lib/timer.hpp>
 #include <nano/node/nodeconfig.hpp>
 #include <nano/node/signatures.hpp>
 #include <nano/node/state_block_signature_verification.hpp>