From 5c96beaf9084416fdcb5f5fa8e83b379e0bbbf01 Mon Sep 17 00:00:00 2001 From: lo-simon Date: Fri, 18 Jul 2025 19:19:42 +0100 Subject: [PATCH 1/6] Prevent exception in the reverse_lock_guard destructor --- Development/nmos/thread_utils.h | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/Development/nmos/thread_utils.h b/Development/nmos/thread_utils.h index b6f7c0d0f..7cf9f32b7 100644 --- a/Development/nmos/thread_utils.h +++ b/Development/nmos/thread_utils.h @@ -15,7 +15,21 @@ namespace nmos public: typedef BasicLockable mutex_type; explicit reverse_lock_guard(mutex_type& m) : m(m) { m.unlock(); } - ~reverse_lock_guard() { m.lock(); } + ~reverse_lock_guard() + { + for (;;) + { + try + { + m.lock(); + break; + } + catch (...) + { + // ignore exception + } + } + } reverse_lock_guard(const reverse_lock_guard&) = delete; reverse_lock_guard& operator=(const reverse_lock_guard&) = delete; private: From 369a12db2a87ceef8ba6f3011520141e7e7aeeb7 Mon Sep 17 00:00:00 2001 From: lo-simon Date: Fri, 25 Jul 2025 16:27:12 +0100 Subject: [PATCH 2/6] Add a try-catch block around the model.write_lock() to handle lock exceptions --- Development/nmos/events_ws_api.cpp | 45 ++++++++++++++++++--------- Development/nmos/registration_api.cpp | 45 ++++++++++++++++++--------- Development/nmos/thread_utils.h | 12 +++++-- 3 files changed, 69 insertions(+), 33 deletions(-) diff --git a/Development/nmos/events_ws_api.cpp b/Development/nmos/events_ws_api.cpp index 146bc3772..9d47b3487 100644 --- a/Development/nmos/events_ws_api.cpp +++ b/Development/nmos/events_ws_api.cpp @@ -504,27 +504,42 @@ namespace nmos // otherwise, there's actually work to do... details::reverse_lock_guard unlock(lock); - // note, without atomic upgrade, another thread may preempt hence the need to recalculate expire_health/forget_health and least_health - auto upgrade = model.write_lock(); + // note 1, without atomic upgrade, another thread may preempt hence the need to recalculate expire_health/forget_health and least_health + // note 2, the try-catch block is used here because the Windows version of the `boost::shared_mutex::lock` throws lock exceptions when + // it has reached the maximum number of 128 exclusive_waiting locks. Another implementation could be replacing the initial + // model.read_lock() with model.write_lock(). Then, we could remove the reverse_lock_guard for switching from read_lock to write_lock. + for (;;) + { + try + { + auto upgrade = model.write_lock(); - expire_health = health_now() - nmos::fields::events_expiry_interval(model.settings); - forget_health = expire_health - nmos::fields::events_expiry_interval(model.settings); + expire_health = health_now() - nmos::fields::events_expiry_interval(model.settings); + forget_health = expire_health - nmos::fields::events_expiry_interval(model.settings); - // forget all resources expired in the previous interval - forget_erased_resources(resources, forget_health); + // forget all resources expired in the previous interval + forget_erased_resources(resources, forget_health); - // expire all connections for which there hasn't been a heartbeat in the last expiry interval - const auto expired = erase_expired_resources(resources, expire_health, false, true); + // expire all connections for which there hasn't been a heartbeat in the last expiry interval + const auto expired = erase_expired_resources(resources, expire_health, false, true); - if (0 != expired) - { - slog::log(gate, SLOG_FLF) << expired << " resources have expired"; + if (0 != expired) + { + slog::log(gate, SLOG_FLF) << expired << " resources have expired"; - slog::log(gate, SLOG_FLF) << "Notifying events websockets thread"; // and anyone else who cares... - model.notify(); - } + slog::log(gate, SLOG_FLF) << "Notifying events websockets thread"; // and anyone else who cares... + model.notify(); + } - least_health = nmos::least_health(resources); + least_health = nmos::least_health(resources); + break; + } + catch (const std::exception& e) + { + slog::log(gate, SLOG_FLF) << "erase_expired_events_resources_thread error: " << e.what(); + std::this_thread::yield(); + } + } } } } diff --git a/Development/nmos/registration_api.cpp b/Development/nmos/registration_api.cpp index 40bd3c50c..5fef68c75 100644 --- a/Development/nmos/registration_api.cpp +++ b/Development/nmos/registration_api.cpp @@ -44,27 +44,42 @@ namespace nmos // otherwise, there's actually work to do... details::reverse_lock_guard unlock(lock); - // note, without atomic upgrade, another thread may preempt hence the need to recalculate expire_health/forget_health and least_health - auto upgrade = model.write_lock(); + // note 1, without atomic upgrade, another thread may preempt hence the need to recalculate expire_health/forget_health and least_health + // note 2, the try-catch block is used here because the Windows version of the `boost::shared_mutex::lock` throws lock exceptions when + // it has reached the maximum number of 128 exclusive_waiting locks. Another implementation could be replacing the initial + // model.read_lock() with model.write_lock(). Then, we could remove the reverse_lock_guard for switching from read_lock to write_lock. + for (;;) + { + try + { + auto upgrade = model.write_lock(); - expire_health = health_now() - nmos::fields::registration_expiry_interval(model.settings); - forget_health = expire_health - nmos::fields::registration_expiry_interval(model.settings); + expire_health = health_now() - nmos::fields::registration_expiry_interval(model.settings); + forget_health = expire_health - nmos::fields::registration_expiry_interval(model.settings); - // forget all resources expired in the previous interval - forget_erased_resources(resources, forget_health); + // forget all resources expired in the previous interval + forget_erased_resources(resources, forget_health); - // expire all nodes for which there hasn't been a heartbeat in the last expiry interval - const auto expired = erase_expired_resources(resources, expire_health, false); + // expire all nodes for which there hasn't been a heartbeat in the last expiry interval + const auto expired = erase_expired_resources(resources, expire_health, false); - if (0 != expired) - { - slog::log(gate, SLOG_FLF) << expired << " resources have expired"; + if (0 != expired) + { + slog::log(gate, SLOG_FLF) << expired << " resources have expired"; - slog::log(gate, SLOG_FLF) << "Notifying query websockets thread"; // and anyone else who cares... - model.notify(); - } + slog::log(gate, SLOG_FLF) << "Notifying query websockets thread"; // and anyone else who cares... + model.notify(); + } - least_health = nmos::least_health(resources); + least_health = nmos::least_health(resources); + break; + } + catch (const std::exception& e) + { + slog::log(gate, SLOG_FLF) << "erase_expired_resources_thread error: " << e.what(); + std::this_thread::yield(); + } + } } } diff --git a/Development/nmos/thread_utils.h b/Development/nmos/thread_utils.h index 7cf9f32b7..2dd002edc 100644 --- a/Development/nmos/thread_utils.h +++ b/Development/nmos/thread_utils.h @@ -17,6 +17,10 @@ namespace nmos explicit reverse_lock_guard(mutex_type& m) : m(m) { m.unlock(); } ~reverse_lock_guard() { + // note, the try-catch block is used here because the Windows version of the `boost::shared_mutex::lock` throws lock exceptions when + // it has reached the maximum number of 128 exclusive_waiting locks. + + // ensure the lock is grabbed before return for (;;) { try @@ -26,7 +30,8 @@ namespace nmos } catch (...) { - // ignore exception + // ignore exception, and try again + std::this_thread::yield(); } } } @@ -43,8 +48,8 @@ namespace nmos { for (;;) { - // Note: the try-catch block is here because Windows boost::condition_variable_any::wait can throw - // an exception once boost::shared_mutex has reached the maximum number of exclusive_waiting locks + // note, the try-catch block is used here because Windows boost::condition_variable_any::wait throws + // lock exception when boost::shared_mutex has reached the maximum number of 128 exclusive_waiting locks try { if ((TimePoint::max)() == tp) @@ -60,6 +65,7 @@ namespace nmos catch (...) { // try the wait again + std::this_thread::yield(); } } } From 471ee55bbac8f28545ea9391aea85610b9acdeca Mon Sep 17 00:00:00 2001 From: lo-simon Date: Fri, 25 Jul 2025 16:28:48 +0100 Subject: [PATCH 3/6] Remove unneceesary try-catch block --- Development/nmos/query_ws_api.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/Development/nmos/query_ws_api.cpp b/Development/nmos/query_ws_api.cpp index 5a31c1880..2304717a5 100644 --- a/Development/nmos/query_ws_api.cpp +++ b/Development/nmos/query_ws_api.cpp @@ -327,14 +327,7 @@ namespace nmos } // send the messages without the lock on resources - try - { - details::reverse_lock_guard unlock{ lock }; - } - catch (const std::exception& e) - { - slog::log(gate, SLOG_FLF) << "Unlock error: " << e.what(); - } + details::reverse_lock_guard unlock{ lock }; if (!outgoing_messages.empty()) slog::log(gate, SLOG_FLF) << "Sending " << outgoing_messages.size() << " websocket messages"; From 73ba85dff389ee70e83ae06a334ec99b2883ae16 Mon Sep 17 00:00:00 2001 From: Simon Lo Date: Tue, 29 Jul 2025 11:07:05 +0100 Subject: [PATCH 4/6] Update Development/nmos/events_ws_api.cpp Co-authored-by: jonathan-r-thorpe <64410119+jonathan-r-thorpe@users.noreply.github.com> --- Development/nmos/events_ws_api.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Development/nmos/events_ws_api.cpp b/Development/nmos/events_ws_api.cpp index 9d47b3487..bd9032b77 100644 --- a/Development/nmos/events_ws_api.cpp +++ b/Development/nmos/events_ws_api.cpp @@ -506,7 +506,7 @@ namespace nmos details::reverse_lock_guard unlock(lock); // note 1, without atomic upgrade, another thread may preempt hence the need to recalculate expire_health/forget_health and least_health // note 2, the try-catch block is used here because the Windows version of the `boost::shared_mutex::lock` throws lock exceptions when - // it has reached the maximum number of 128 exclusive_waiting locks. Another implementation could be replacing the initial + // it has reached the maximum number of 128 exclusive_waiting locks. As an alternative we could replace the initial // model.read_lock() with model.write_lock(). Then, we could remove the reverse_lock_guard for switching from read_lock to write_lock. for (;;) { From 17488db3b2f5c727692b276215e3150ae768144f Mon Sep 17 00:00:00 2001 From: Simon Lo Date: Tue, 29 Jul 2025 11:07:14 +0100 Subject: [PATCH 5/6] Update Development/nmos/registration_api.cpp Co-authored-by: jonathan-r-thorpe <64410119+jonathan-r-thorpe@users.noreply.github.com> --- Development/nmos/registration_api.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Development/nmos/registration_api.cpp b/Development/nmos/registration_api.cpp index 5fef68c75..ba02467a4 100644 --- a/Development/nmos/registration_api.cpp +++ b/Development/nmos/registration_api.cpp @@ -46,7 +46,7 @@ namespace nmos details::reverse_lock_guard unlock(lock); // note 1, without atomic upgrade, another thread may preempt hence the need to recalculate expire_health/forget_health and least_health // note 2, the try-catch block is used here because the Windows version of the `boost::shared_mutex::lock` throws lock exceptions when - // it has reached the maximum number of 128 exclusive_waiting locks. Another implementation could be replacing the initial + // it has reached the maximum number of 128 exclusive_waiting locks. As an alternative we could replace the initial // model.read_lock() with model.write_lock(). Then, we could remove the reverse_lock_guard for switching from read_lock to write_lock. for (;;) { From a5ab7291a3e82c428e4ec6621f010d55e72e94c3 Mon Sep 17 00:00:00 2001 From: lo-simon Date: Thu, 31 Jul 2025 16:34:06 +0100 Subject: [PATCH 6/6] Show a more descriptive boost lock error message on the HTTP response --- Development/nmos/api_utils.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Development/nmos/api_utils.cpp b/Development/nmos/api_utils.cpp index aab46a16c..64a20bde6 100644 --- a/Development/nmos/api_utils.cpp +++ b/Development/nmos/api_utils.cpp @@ -653,6 +653,12 @@ namespace nmos slog::log(gate, SLOG_FLF) << "HTTP error: " << e.what() << " [" << e.error_code() << "]"; set_error_reply(res, status_codes::BadRequest, e); } + // Boost lock error indicates cannot get lock, perhaps lock limit exceeded + catch (const boost::lock_error& e) + { + slog::log(gate, SLOG_FLF) << "Boost lock error: " << e.what() << " Perhaps lock limit exceeded"; + set_error_reply(res, status_codes::ServiceUnavailable, {}, U("Cannot get lock. Perhaps lock limit exceeded")); + } // while a runtime_error (often) indicates an unimplemented feature catch (const std::runtime_error& e) {