From 67c2e7a67e067a9f7806767f2697c76828262429 Mon Sep 17 00:00:00 2001 From: Erio-Harrison Date: Sun, 7 Dec 2025 10:33:38 +1100 Subject: [PATCH 1/2] refactor: replace std::lock+lock_guard with std::scoped_lock (C++17) --- src/edge_node.cpp | 6 ++---- src/host_application.cpp | 5 ++--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/edge_node.cpp b/src/edge_node.cpp index 65f5657..e9681de 100644 --- a/src/edge_node.cpp +++ b/src/edge_node.cpp @@ -158,10 +158,8 @@ EdgeNode::EdgeNode(EdgeNode&& other) noexcept EdgeNode& EdgeNode::operator=(EdgeNode&& other) noexcept { if (this != &other) { - // Lock both mutexes in consistent order to avoid deadlock - std::lock(mutex_, other.mutex_); - std::lock_guard lock1(mutex_, std::adopt_lock); - std::lock_guard lock2(other.mutex_, std::adopt_lock); + // Lock both mutexes with automatic deadlock avoidance + std::scoped_lock lock(mutex_, other.mutex_); config_ = std::move(other.config_); client_ = std::move(other.client_); diff --git a/src/host_application.cpp b/src/host_application.cpp index 85cff63..f0f29e0 100644 --- a/src/host_application.cpp +++ b/src/host_application.cpp @@ -70,9 +70,8 @@ HostApplication::HostApplication(HostApplication&& other) noexcept HostApplication& HostApplication::operator=(HostApplication&& other) noexcept { if (this != &other) { - std::lock(mutex_, other.mutex_); - std::lock_guard lock1(mutex_, std::adopt_lock); - std::lock_guard lock2(other.mutex_, std::adopt_lock); + // Lock both mutexes with automatic deadlock avoidance + std::scoped_lock lock(mutex_, other.mutex_); config_ = std::move(other.config_); client_ = std::move(other.client_); From 8da1778810f87cc64c94d373f1cf3be467c34d18 Mon Sep 17 00:00:00 2001 From: Erio-Harrison Date: Sun, 7 Dec 2025 11:10:38 +1100 Subject: [PATCH 2/2] refactor: migrate all mutex locks to std::scoped_lock (C++17) --- examples/tck_edge_node.cpp | 2 +- examples/tck_host_application.cpp | 2 +- examples/tck_test_runner.cpp | 4 +-- include/sparkplug/edge_node.hpp | 6 ++--- src/edge_node.cpp | 44 +++++++++++++++---------------- src/host_application.cpp | 38 +++++++++++++------------- 6 files changed, 48 insertions(+), 48 deletions(-) diff --git a/examples/tck_edge_node.cpp b/examples/tck_edge_node.cpp index 15348b1..be53b64 100644 --- a/examples/tck_edge_node.cpp +++ b/examples/tck_edge_node.cpp @@ -1088,7 +1088,7 @@ auto TCKEdgeNode::create_edge_node(const std::string& host_id, const std::string& edge_node_id, const std::vector& device_ids) -> stdx::expected { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (edge_node_) { return stdx::unexpected("Edge Node already exists"); diff --git a/examples/tck_host_application.cpp b/examples/tck_host_application.cpp index 22708c3..266240f 100644 --- a/examples/tck_host_application.cpp +++ b/examples/tck_host_application.cpp @@ -305,7 +305,7 @@ void TCKHostApplication::handle_prompt_specific(const std::string& message) { auto TCKHostApplication::establish_session(const std::string& host_id) -> stdx::expected { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (host_application_) { return {}; diff --git a/examples/tck_test_runner.cpp b/examples/tck_test_runner.cpp index 027d5f0..3761794 100644 --- a/examples/tck_test_runner.cpp +++ b/examples/tck_test_runner.cpp @@ -52,7 +52,7 @@ TCKTestRunner::~TCKTestRunner() { } auto TCKTestRunner::start() -> stdx::expected { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (running_) { return stdx::unexpected("TCK test runner is already running"); @@ -91,7 +91,7 @@ auto TCKTestRunner::start() -> stdx::expected { } void TCKTestRunner::stop() { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!running_) { return; diff --git a/include/sparkplug/edge_node.hpp b/include/sparkplug/edge_node.hpp index 9dee2b4..98f902a 100644 --- a/include/sparkplug/edge_node.hpp +++ b/include/sparkplug/edge_node.hpp @@ -284,7 +284,7 @@ class EdgeNode { * @note Useful for monitoring and debugging. */ [[nodiscard]] uint64_t get_seq() const { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); return seq_num_; } @@ -296,7 +296,7 @@ class EdgeNode { * @note Used by SCADA to detect new sessions/rebirths. */ [[nodiscard]] uint64_t get_bd_seq() const { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); return bd_seq_num_; } @@ -310,7 +310,7 @@ class EdgeNode { * @note Used to determine if NBIRTH/DBIRTH can be published. */ [[nodiscard]] bool is_primary_host_online() const { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); return primary_host_online_; } diff --git a/src/edge_node.cpp b/src/edge_node.cpp index e9681de..cdae4d5 100644 --- a/src/edge_node.cpp +++ b/src/edge_node.cpp @@ -62,7 +62,7 @@ void EdgeNode::on_connection_lost(void* context, char* cause) { } { - std::lock_guard lock(edge_node->mutex_); + std::scoped_lock lock(edge_node->mutex_); edge_node->is_connected_ = false; } @@ -101,7 +101,7 @@ int EdgeNode::on_message_arrived(void* context, std::string payload_str(static_cast(message->payload), message->payloadlen); - std::lock_guard lock(edge_node->mutex_); + std::scoped_lock lock(edge_node->mutex_); if (payload_str.find("\"online\":true") != std::string::npos) { edge_node->primary_host_online_ = true; } else if (payload_str.find("\"online\":false") != std::string::npos) { @@ -152,7 +152,7 @@ EdgeNode::EdgeNode(EdgeNode&& other) noexcept device_states_(std::move(other.device_states_)), is_connected_(other.is_connected_) // mutex_ is default-constructed (mutexes are not moveable) { - std::lock_guard lock(other.mutex_); + std::scoped_lock lock(other.mutex_); other.is_connected_ = false; } @@ -176,23 +176,23 @@ EdgeNode& EdgeNode::operator=(EdgeNode&& other) noexcept { void EdgeNode::set_credentials(std::optional username, std::optional password) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); config_.username = std::move(username); config_.password = std::move(password); } void EdgeNode::set_tls(std::optional tls) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); config_.tls = std::move(tls); } void EdgeNode::set_log_callback(std::optional callback) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); config_.log_callback = std::move(callback); } stdx::expected EdgeNode::connect() { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); MQTTAsync raw_client = nullptr; int rc = @@ -359,7 +359,7 @@ stdx::expected EdgeNode::connect() { } stdx::expected EdgeNode::disconnect() { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!client_) { return stdx::unexpected("Not connected"); @@ -428,7 +428,7 @@ stdx::expected EdgeNode::publish_birth(PayloadBuilder& payloa int qos = 0; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -477,7 +477,7 @@ stdx::expected EdgeNode::publish_birth(PayloadBuilder& payloa } { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); last_birth_payload_ = std::move(payload_data); seq_num_ = 0; } @@ -492,7 +492,7 @@ stdx::expected EdgeNode::publish_data(PayloadBuilder& payload int qos = 0; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -525,7 +525,7 @@ stdx::expected EdgeNode::publish_death() { int qos = 0; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -565,7 +565,7 @@ stdx::expected EdgeNode::rebirth() { int qos = 0; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -617,7 +617,7 @@ stdx::expected EdgeNode::rebirth() { .and_then([this, &topic_str, &payload_data, qos]() { MQTTAsync client = nullptr; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); client = client_.get(); } return publish_message(client, topic_str, payload_data, qos, false); @@ -628,7 +628,7 @@ stdx::expected EdgeNode::rebirth() { } { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); seq_num_ = 0; } @@ -643,7 +643,7 @@ EdgeNode::publish_device_birth(std::string_view device_id, PayloadBuilder& paylo int qos = 0; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -711,7 +711,7 @@ EdgeNode::publish_device_birth(std::string_view device_id, PayloadBuilder& paylo } { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); auto& device_state = device_states_[std::string(device_id)]; device_state.last_birth_payload = std::move(payload_data); device_state.is_online = true; @@ -728,7 +728,7 @@ EdgeNode::publish_device_data(std::string_view device_id, PayloadBuilder& payloa int qos = 0; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -768,7 +768,7 @@ EdgeNode::publish_device_death(std::string_view device_id) { int qos = 0; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -804,7 +804,7 @@ EdgeNode::publish_device_death(std::string_view device_id) { } { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); auto it = device_states_.find(device_id); if (it != device_states_.end()) { it->second.is_online = false; @@ -823,7 +823,7 @@ EdgeNode::publish_node_command(std::string_view target_edge_node_id, int qos = 0; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -853,7 +853,7 @@ EdgeNode::publish_device_command(std::string_view target_edge_node_id, int qos = 0; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); diff --git a/src/host_application.cpp b/src/host_application.cpp index f0f29e0..012c85a 100644 --- a/src/host_application.cpp +++ b/src/host_application.cpp @@ -64,7 +64,7 @@ HostApplication::~HostApplication() { HostApplication::HostApplication(HostApplication&& other) noexcept : config_(std::move(other.config_)), client_(std::move(other.client_)), is_connected_(other.is_connected_) { - std::lock_guard lock(other.mutex_); + std::scoped_lock lock(other.mutex_); other.is_connected_ = false; } @@ -83,28 +83,28 @@ HostApplication& HostApplication::operator=(HostApplication&& other) noexcept { void HostApplication::set_credentials(std::optional username, std::optional password) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); config_.username = std::move(username); config_.password = std::move(password); } void HostApplication::set_tls(std::optional tls) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); config_.tls = std::move(tls); } void HostApplication::set_message_callback(MessageCallback callback) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); config_.message_callback = std::move(callback); } void HostApplication::set_log_callback(LogCallback callback) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); config_.log_callback = std::move(callback); } stdx::expected HostApplication::connect() { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); MQTTAsync raw_client = nullptr; int rc = @@ -181,7 +181,7 @@ stdx::expected HostApplication::connect() { } stdx::expected HostApplication::disconnect() { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!client_) { return stdx::unexpected("Not connected"); @@ -219,7 +219,7 @@ stdx::expected HostApplication::disconnect() { stdx::expected HostApplication::publish_state_birth(uint64_t timestamp) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -237,7 +237,7 @@ HostApplication::publish_state_birth(uint64_t timestamp) { stdx::expected HostApplication::publish_state_death(uint64_t timestamp) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -260,7 +260,7 @@ HostApplication::publish_node_command(std::string_view group_id, std::string topic_str; std::vector payload_data; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -286,7 +286,7 @@ HostApplication::publish_device_command(std::string_view group_id, std::string topic_str; std::vector payload_data; { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!is_connected_) { return stdx::unexpected("Not connected"); @@ -378,7 +378,7 @@ HostApplication::publish_command_message(std::string_view topic, } stdx::expected HostApplication::subscribe_all_groups() { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!client_) { return stdx::unexpected("Not connected"); @@ -398,7 +398,7 @@ stdx::expected HostApplication::subscribe_all_groups() { stdx::expected HostApplication::subscribe_group(std::string_view group_id) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!client_) { return stdx::unexpected("Not connected"); @@ -419,7 +419,7 @@ HostApplication::subscribe_group(std::string_view group_id) { stdx::expected HostApplication::subscribe_node(std::string_view group_id, std::string_view edge_node_id) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!client_) { return stdx::unexpected("Not connected"); @@ -439,7 +439,7 @@ HostApplication::subscribe_node(std::string_view group_id, stdx::expected HostApplication::subscribe_state(std::string_view host_id) { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); if (!client_) { return stdx::unexpected("Not connected"); @@ -460,7 +460,7 @@ HostApplication::subscribe_state(std::string_view host_id) { std::optional> HostApplication::get_node_state(std::string_view group_id, std::string_view edge_node_id) const { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); auto it = node_states_.find(std::make_pair(group_id, edge_node_id)); if (it != node_states_.end()) { @@ -474,7 +474,7 @@ HostApplication::get_metric_name(std::string_view group_id, std::string_view edge_node_id, std::string_view device_id, uint64_t alias) const { - std::lock_guard lock(mutex_); + std::scoped_lock lock(mutex_); auto it = node_states_.find(std::make_pair(group_id, edge_node_id)); if (it == node_states_.end()) { @@ -754,7 +754,7 @@ int HostApplication::on_message_arrived(void* context, } { - std::lock_guard lock(host_app->mutex_); + std::scoped_lock lock(host_app->mutex_); host_app->validate_message(*topic_result, payload); } @@ -777,7 +777,7 @@ void HostApplication::on_connection_lost(void* context, char* cause) { } { - std::lock_guard lock(host_app->mutex_); + std::scoped_lock lock(host_app->mutex_); host_app->is_connected_ = false; }