From 6de5080bfaf8320f65a3074a10de6ed62b3d5acc Mon Sep 17 00:00:00 2001 From: jim Date: Sun, 10 Jul 2022 11:42:02 +0800 Subject: [PATCH 1/3] fix: liveliness core dump caused by omit mutex lock --- src/cpp/rtps/writer/LivelinessManager.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cpp/rtps/writer/LivelinessManager.cpp b/src/cpp/rtps/writer/LivelinessManager.cpp index 3d6425959a0..67cbc79319f 100644 --- a/src/cpp/rtps/writer/LivelinessManager.cpp +++ b/src/cpp/rtps/writer/LivelinessManager.cpp @@ -288,8 +288,10 @@ bool LivelinessManager::assert_liveliness( bool LivelinessManager::calculate_next() { + std::unique_lock lock(mutex_); timer_owner_ = nullptr; - + lock.unlock(); + steady_clock::time_point min_time = steady_clock::now() + nanoseconds(c_TimeInfinite.to_ns()); bool any_alive = false; From 4724a2fa7cbf95e25b0ccf2ace7e0d1d5e8f7779 Mon Sep 17 00:00:00 2001 From: Miguel Barro Date: Mon, 11 Jul 2022 11:18:06 +0200 Subject: [PATCH 2/3] Refactor that includes the fix and speeds things up Signed-off-by: Miguel Barro --- src/cpp/rtps/writer/LivelinessManager.cpp | 31 ++++++++++------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/cpp/rtps/writer/LivelinessManager.cpp b/src/cpp/rtps/writer/LivelinessManager.cpp index 67cbc79319f..512fe5cbfb6 100644 --- a/src/cpp/rtps/writer/LivelinessManager.cpp +++ b/src/cpp/rtps/writer/LivelinessManager.cpp @@ -33,7 +33,7 @@ LivelinessManager::LivelinessManager( LivelinessManager::~LivelinessManager() { - std::unique_lock lock(mutex_); + std::lock_guard _(mutex_); timer_owner_ = nullptr; timer_.cancel_timer(); } @@ -52,12 +52,11 @@ bool LivelinessManager::add_writer( { // collection guard std::lock_guard _(col_mutex_); + // writers_ elements guard + std::lock_guard __(mutex_); for (LivelinessData& writer : writers_) { - // writers_ elements guard - std::lock_guard __(mutex_); - if (writer.guid == guid && writer.kind == kind && writer.lease_duration == lease_duration) @@ -101,11 +100,11 @@ bool LivelinessManager::remove_writer( { // collection guard std::lock_guard _(col_mutex_); + // writers_ elements guard + std::lock_guard __(mutex_); removed = writers_.remove_if([guid, kind, lease_duration, &status, this](LivelinessData& writer) { - // writers_ elements guard - std::lock_guard _(mutex_); status = writer.status; return writer.guid == guid && writer.kind == kind && @@ -288,22 +287,18 @@ bool LivelinessManager::assert_liveliness( bool LivelinessManager::calculate_next() { - std::unique_lock lock(mutex_); - timer_owner_ = nullptr; - lock.unlock(); - - steady_clock::time_point min_time = steady_clock::now() + nanoseconds(c_TimeInfinite.to_ns()); + // Keep this lock order to prevent ABBA deadlocks + shared_lock _(col_mutex_); + std::lock_guard __(mutex_); bool any_alive = false; + steady_clock::time_point min_time = steady_clock::now() + nanoseconds(c_TimeInfinite.to_ns()); - // collection guard - shared_lock _(col_mutex_); + timer_owner_ = nullptr; + // collection guard for (LivelinessData& writer : writers_) { - // writers_ elements guard - std::lock_guard __(mutex_); - if (writer.status == LivelinessData::WriterStatus::ALIVE) { if (writer.time < min_time) @@ -364,12 +359,12 @@ bool LivelinessManager::timer_expired() bool LivelinessManager::is_any_alive( LivelinessQosPolicyKind kind) { + // Keep this lock order to prevent ABBA deadlocks shared_lock _(col_mutex_); + std::lock_guard __(mutex_); for (const auto& writer : writers_) { - std::unique_lock lock(mutex_); - if (writer.kind == kind && writer.status == LivelinessData::WriterStatus::ALIVE) { return true; From 6aa041ec20cc90cff6f4f0fea7a5e11f56fcab7e Mon Sep 17 00:00:00 2001 From: Miguel Barro Date: Wed, 13 Jul 2022 19:39:51 +0200 Subject: [PATCH 3/3] Fixing Clang warnings Signed-off-by: Miguel Barro --- CMakeLists.txt | 3 ++- src/cpp/rtps/writer/LivelinessManager.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1205e93ffa6..d4c1bb8e59a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -62,7 +62,8 @@ if(MSVC OR MSVC_IDE) # C4555 expression has no effect; expected expression with side-effect # C4715: 'Test': not all control paths return a value # C5038 data member 'member1' will be initialized after data member 'member2' - add_compile_options(/w34101 /w34189 /w34555 /w34715 /w35038) + # C4100 'identifier' : unreferenced formal parameter (matches clang -Wunused-lambda-capture) + add_compile_options(/w34101 /w34189 /w34555 /w34715 /w35038 /w44100) if(EPROSIMA_BUILD) string(REPLACE "/DNDEBUG" "" CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO}") diff --git a/src/cpp/rtps/writer/LivelinessManager.cpp b/src/cpp/rtps/writer/LivelinessManager.cpp index 512fe5cbfb6..bfd79abb2cd 100644 --- a/src/cpp/rtps/writer/LivelinessManager.cpp +++ b/src/cpp/rtps/writer/LivelinessManager.cpp @@ -103,7 +103,7 @@ bool LivelinessManager::remove_writer( // writers_ elements guard std::lock_guard __(mutex_); - removed = writers_.remove_if([guid, kind, lease_duration, &status, this](LivelinessData& writer) + removed = writers_.remove_if([guid, kind, lease_duration, &status](LivelinessData& writer) { status = writer.status; return writer.guid == guid &&