Fix TSAN race condition in FileLogger2 using base-from-member idiom

The root cause was that StatusChangeLogger subscribes to callbacks in
its constructor, but _p (the PImpl) wasn't initialized until after the
base class constructor completed. This meant callbacks could fire with
_p uninitialized.

Solution: Use the base-from-member idiom to ensure _p is initialized
BEFORE StatusChangeLogger. C++ initializes base classes in declaration
order, so by inheriting from FileLogger2PImplBase before StatusChangeLogger,
we guarantee _p exists when callbacks start firing.

Also wait for writer thread to be ready using condition_variable
synchronization before returning from the constructor.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Author: facontidavide

include/behaviortree_cpp/loggers/bt_file_logger_v2.h

@@ -8,6 +8,25 @@
 
 namespace BT
 {
+
+// Forward declaration for base-from-member idiom
+namespace detail
+{
+struct FileLogger2Private;
+
+// This base class ensures _p is initialized BEFORE StatusChangeLogger,
+// which subscribes to callbacks in its constructor. Without this,
+// callbacks could fire before _p exists (C++ initializes bases before members).
+struct FileLogger2PImplBase
+{
+  std::unique_ptr<FileLogger2Private> _p;
+  FileLogger2PImplBase();
+  ~FileLogger2PImplBase();
+  FileLogger2PImplBase(FileLogger2PImplBase&&) = default;
+  FileLogger2PImplBase& operator=(FileLogger2PImplBase&&) = default;
+};
+}  // namespace detail
+
 /**
  * @brief The FileLogger2 is a logger that saves the tree as
  * XML and all the transitions.
@@ -21,7 +40,7 @@ namespace BT
  * - next: each 9 bytes is a FileLogger2::Transition. See definition.
  *
  */
-class FileLogger2 : public StatusChangeLogger
+class FileLogger2 : private detail::FileLogger2PImplBase, public StatusChangeLogger
 {
 public:
   /**
@@ -36,8 +55,8 @@ class FileLogger2 : public StatusChangeLogger
   FileLogger2(const FileLogger2& other) = delete;
   FileLogger2& operator=(const FileLogger2& other) = delete;
 
-  FileLogger2(FileLogger2&& other) = default;
-  FileLogger2& operator=(FileLogger2&& other) = default;
+  FileLogger2(FileLogger2&& other) = delete;
+  FileLogger2& operator=(FileLogger2&& other) = delete;
 
   virtual ~FileLogger2() override;
 
@@ -58,9 +77,6 @@ class FileLogger2 : public StatusChangeLogger
   void flush() override;
 
 private:
-  struct PImpl;
-  std::unique_ptr<PImpl> _p;
-
   void writerLoop();
 };
 

src/loggers/bt_file_logger_v2.cpp

@@ -15,13 +15,14 @@ int64_t ToUsec(Duration ts)
 }
 }  // namespace
 
-struct FileLogger2::PImpl
+// Define the private implementation struct
+struct detail::FileLogger2Private
 {
   std::ofstream file_stream;
 
   Duration first_timestamp = {};
 
-  std::deque<Transition> transitions_queue;
+  std::deque<FileLogger2::Transition> transitions_queue;
   std::condition_variable queue_cv;
   std::mutex queue_mutex;
 
@@ -34,9 +35,22 @@ struct FileLogger2::PImpl
   bool ready = false;
 };
 
+// Base class constructor - initializes _p before StatusChangeLogger
+detail::FileLogger2PImplBase::FileLogger2PImplBase()
+  : _p(std::make_unique<FileLogger2Private>())
+{}
+
+// Destructor must be defined where FileLogger2Private is complete
+detail::FileLogger2PImplBase::~FileLogger2PImplBase() = default;
+
 FileLogger2::FileLogger2(const BT::Tree& tree, std::filesystem::path const& filepath)
-  : StatusChangeLogger(tree.rootNode()), _p(new PImpl)
+  : StatusChangeLogger(tree.rootNode())
 {
+  // Note: _p is already initialized by FileLogger2PImplBase before
+  // StatusChangeLogger's constructor runs. This is critical because
+  // StatusChangeLogger subscribes to callbacks in its constructor,
+  // and those callbacks may access _p.
+
   if(filepath.filename().extension() != ".btlog")
   {
     throw RuntimeError("FileLogger2: the file extension must be [.btlog]");

tests/gtest_decorator.cpp

@@ -20,12 +20,12 @@
 using BT::NodeStatus;
 using std::chrono::milliseconds;
 
-// Timing constants for faster test execution
-constexpr int DEADLINE_MS = 30;  // Timeout threshold
+// Timing constants - need generous margins for Windows timer resolution (~15.6ms)
+constexpr int DEADLINE_MS = 100;  // Timeout threshold
 constexpr auto ACTION_LONG_MS =
-    milliseconds(50);  // Action longer than deadline (will timeout)
+    milliseconds(150);  // Action longer than deadline (will timeout)
 constexpr auto ACTION_SHORT_MS =
-    milliseconds(20);  // Action shorter than deadline (will succeed)
+    milliseconds(30);  // Action shorter than deadline (will succeed)
 
 struct DeadlineTest : testing::Test
 {