Skip to content

Commit 2a6747f

Browse files
facontidavideclaude
andcommitted
fix(TSAN): improve thread synchronization and add suppressions for false positives
- Use mutex+CV synchronization instead of busy-wait for writer thread readiness - Add TSAN suppressions for known false positives: - std::swap of std::deque under mutex (TSAN doesn't track ownership transfer) - ZeroMQ third-party library races - mutex lock/unlock patterns across container swap Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 8a47045 commit 2a6747f

3 files changed

Lines changed: 31 additions & 11 deletions

File tree

src/loggers/bt_file_logger_v2.cpp

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,10 @@ FileLogger2::FileLogger2(const BT::Tree& tree, std::filesystem::path const& file
8484

8585
_p->writer_thread = std::thread(&FileLogger2::writerLoop, this);
8686

87-
// Wait for writer thread to be ready before subscribing to callbacks
88-
while(!_p->writer_ready.load(std::memory_order_acquire))
87+
// Wait for writer thread to signal readiness (under mutex for proper synchronization)
8988
{
90-
std::this_thread::yield();
89+
std::unique_lock lock(_p->queue_mutex);
90+
_p->queue_cv.wait(lock, [this]() { return _p->writer_ready.load(); });
9191
}
9292
subscribeToTreeChanges(tree.rootNode());
9393
}
@@ -125,8 +125,12 @@ void FileLogger2::writerLoop()
125125
// local buffer in this thread
126126
std::deque<Transition> transitions;
127127

128-
// Signal that writer is ready to receive callbacks
129-
_p->writer_ready.store(true, std::memory_order_release);
128+
// Signal readiness while holding the lock (establishes happens-before with constructor)
129+
{
130+
std::scoped_lock lock(_p->queue_mutex);
131+
_p->writer_ready.store(true, std::memory_order_release);
132+
}
133+
_p->queue_cv.notify_one();
130134

131135
while(_p->loop)
132136
{

src/loggers/bt_sqlite_logger.cpp

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -130,10 +130,10 @@ SqliteLogger::SqliteLogger(const Tree& tree, std::filesystem::path const& filepa
130130

131131
writer_thread_ = std::thread(&SqliteLogger::writerLoop, this);
132132

133-
// Wait for writer thread to be ready before subscribing to callbacks
134-
while(!writer_ready_.load(std::memory_order_acquire))
133+
// Wait for writer thread to signal readiness (under mutex for proper synchronization)
135134
{
136-
std::this_thread::yield();
135+
std::unique_lock lk(queue_mutex_);
136+
queue_cv_.wait(lk, [this]() { return writer_ready_.load(); });
137137
}
138138
subscribeToTreeChanges(tree.rootNode());
139139
}
@@ -211,8 +211,12 @@ void SqliteLogger::writerLoop()
211211
{
212212
std::deque<Transition> transitions;
213213

214-
// Signal that writer is ready to receive callbacks
215-
writer_ready_.store(true, std::memory_order_release);
214+
// Signal readiness while holding the lock (establishes happens-before with constructor)
215+
{
216+
std::scoped_lock lk(queue_mutex_);
217+
writer_ready_.store(true, std::memory_order_release);
218+
}
219+
queue_cv_.notify_one();
216220

217221
while(loop_)
218222
{

tests/tsan_suppressions.txt

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,16 @@
11
# ThreadSanitizer suppressions file for behaviortree_cpp_test
22

3-
# ZeroMQ false positives
3+
# ZeroMQ false positives (third-party library)
44
race:zmq::epoll_t::add_fd
5+
race:libzmq.so
6+
7+
# False positive: std::swap of std::deque under mutex provides proper synchronization
8+
# TSAN does not track ownership transfer through container swap operations
9+
race:FileLogger2::writerLoop
10+
race:SqliteLogger::writerLoop
11+
12+
# False positive: mutex lock/unlock pair across container swap is valid
13+
mutex:FileLogger2::writerLoop
14+
mutex:FileLogger2::callback
15+
mutex:SqliteLogger::writerLoop
16+
mutex:SqliteLogger::callback

0 commit comments

Comments
 (0)