From 33c59b599c39b056171be4c3eda038886c7796b7 Mon Sep 17 00:00:00 2001 From: Dustin Spicuzza Date: Sun, 16 Aug 2026 05:51:47 +0000 Subject: [PATCH 1/2] [datalog] Guard FileLogger callbacks during log destruction --- datalog/src/main/native/cpp/DataLog.cpp | 34 +++++++++++++++++++ .../native/cpp/DataLogBackgroundWriter.cpp | 1 + datalog/src/main/native/cpp/DataLogWriter.cpp | 1 + datalog/src/main/native/cpp/FileLogger.cpp | 5 +-- .../native/include/wpi/datalog/DataLog.hpp | 21 ++++++++++-- .../src/test/native/cpp/FileLoggerTest.cpp | 31 +++++++++++++++++ 6 files changed, 86 insertions(+), 7 deletions(-) diff --git a/datalog/src/main/native/cpp/DataLog.cpp b/datalog/src/main/native/cpp/DataLog.cpp index 589002b1b92..27e9fce4785 100644 --- a/datalog/src/main/native/cpp/DataLog.cpp +++ b/datalog/src/main/native/cpp/DataLog.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -35,6 +36,39 @@ static void DefaultLog(unsigned int level, const char* file, unsigned int line, wpi::util::Logger DataLog::s_defaultMessageLog{DefaultLog}; +struct DataLog::FileLoggerCallbackState { + explicit FileLoggerCallbackState(DataLog* log) : log{log} {} + + wpi::util::mutex mutex; + DataLog* log; +}; + +DataLog::DataLog(wpi::util::Logger& msglog, std::string_view extraHeader) + : m_msglog{msglog}, + m_extraHeader{extraHeader}, + m_fileLoggerCallbackState{ + std::make_shared(this)} {} + +DataLog::~DataLog() { + InvalidateFileLoggerCallbacks(); +} + +void DataLog::InvalidateFileLoggerCallbacks() { + std::scoped_lock lock{m_fileLoggerCallbackState->mutex}; + m_fileLoggerCallbackState->log = nullptr; +} + +std::function DataLog::MakeFileLoggerCallback( + std::string_view key) { + int entry = Start(key, "string"); + return [entry, state = m_fileLoggerCallbackState](std::string_view line) { + std::scoped_lock lock{state->mutex}; + if (state->log) { + state->log->AppendString(entry, line, 0); + } + }; +} + template static unsigned int WriteVarInt(uint8_t* buf, T val) { unsigned int len = 0; diff --git a/datalog/src/main/native/cpp/DataLogBackgroundWriter.cpp b/datalog/src/main/native/cpp/DataLogBackgroundWriter.cpp index 4887f54a09f..ed2da84f0ce 100644 --- a/datalog/src/main/native/cpp/DataLogBackgroundWriter.cpp +++ b/datalog/src/main/native/cpp/DataLogBackgroundWriter.cpp @@ -81,6 +81,7 @@ DataLogBackgroundWriter::DataLogBackgroundWriter( }} {} DataLogBackgroundWriter::~DataLogBackgroundWriter() { + InvalidateFileLoggerCallbacks(); { std::scoped_lock lock{m_mutex}; m_shutdown = true; diff --git a/datalog/src/main/native/cpp/DataLogWriter.cpp b/datalog/src/main/native/cpp/DataLogWriter.cpp index 5b66829b2b2..71ee1c21849 100644 --- a/datalog/src/main/native/cpp/DataLogWriter.cpp +++ b/datalog/src/main/native/cpp/DataLogWriter.cpp @@ -47,6 +47,7 @@ DataLogWriter::DataLogWriter(wpi::util::Logger& msglog, } DataLogWriter::~DataLogWriter() { + InvalidateFileLoggerCallbacks(); if (m_os) { Flush(); } diff --git a/datalog/src/main/native/cpp/FileLogger.cpp b/datalog/src/main/native/cpp/FileLogger.cpp index 754e998fddf..df12e78a258 100644 --- a/datalog/src/main/native/cpp/FileLogger.cpp +++ b/datalog/src/main/native/cpp/FileLogger.cpp @@ -69,10 +69,7 @@ FileLogger::FileLogger(std::string_view file, } FileLogger::FileLogger(std::string_view file, log::DataLog& log, std::string_view key) - : FileLogger(file, Buffer([entry = log.Start(key, "string"), - &log](std::string_view line) { - log.AppendString(entry, line, 0); - })) {} + : FileLogger(file, Buffer(log.MakeFileLoggerCallback(key))) {} FileLogger::FileLogger(FileLogger&& other) #ifdef __linux__ : m_fileHandle{std::exchange(other.m_fileHandle, -1)}, diff --git a/datalog/src/main/native/include/wpi/datalog/DataLog.hpp b/datalog/src/main/native/include/wpi/datalog/DataLog.hpp index ba2138cd01e..d4d5ddd3f76 100644 --- a/datalog/src/main/native/include/wpi/datalog/DataLog.hpp +++ b/datalog/src/main/native/include/wpi/datalog/DataLog.hpp @@ -8,7 +8,9 @@ #include #include +#include #include +#include #include #include #include @@ -34,6 +36,8 @@ class Logger; namespace wpi::log { +class FileLogger; + namespace impl { enum ControlRecordType { @@ -68,7 +72,7 @@ enum ControlRecordType { */ class DataLog { public: - virtual ~DataLog() = default; + virtual ~DataLog(); DataLog(const DataLog&) = delete; DataLog& operator=(const DataLog&) = delete; @@ -441,8 +445,11 @@ class DataLog { * @param msglog message logger (will be called from separate thread) * @param extraHeader extra header metadata */ - explicit DataLog(wpi::util::Logger& msglog, std::string_view extraHeader = "") - : m_msglog{msglog}, m_extraHeader{extraHeader} {} + explicit DataLog(wpi::util::Logger& msglog, + std::string_view extraHeader = ""); + + /** Prevents FileLogger callbacks from accessing this log. */ + void InvalidateFileLoggerCallbacks(); /** * Starts the log. Appends file header and Start records and schema data @@ -487,6 +494,13 @@ class DataLog { virtual bool BufferFull() = 0; private: + friend class FileLogger; + + struct FileLoggerCallbackState; + + std::function MakeFileLoggerCallback( + std::string_view key); + static constexpr size_t kMaxBufferCount = 1024 * 1024 / kBlockSize; static constexpr size_t kMaxFreeCount = 256 * 1024 / kBlockSize; @@ -528,6 +542,7 @@ class DataLog { }; wpi::util::DenseMap m_entryIds; int m_lastId = 0; + std::shared_ptr m_fileLoggerCallbackState; }; /** diff --git a/datalog/src/test/native/cpp/FileLoggerTest.cpp b/datalog/src/test/native/cpp/FileLoggerTest.cpp index c82f1f85d5f..57eaea1f0de 100644 --- a/datalog/src/test/native/cpp/FileLoggerTest.cpp +++ b/datalog/src/test/native/cpp/FileLoggerTest.cpp @@ -6,7 +6,10 @@ #include #include +#include #include +#include +#include #include #include #include @@ -14,6 +17,9 @@ #include +#include "wpi/datalog/DataLogWriter.hpp" +#include "wpi/util/raw_ostream.hpp" + #ifdef __linux__ #include #include @@ -69,6 +75,31 @@ TEST_CASE("FileLoggerTest BufferMultipleMultiLinePartials", } #ifdef __linux__ +TEST_CASE("FileLoggerTest DataLogCanBeDestroyedFirst", + "[datalog][file-logger]") { + auto path = std::filesystem::temp_directory_path() / + std::format("wpi_filelogger_log_lifetime_{}", getpid()); + { + std::ofstream create{path}; + } + + std::vector output; + auto log = std::make_unique( + std::make_unique(output)); + wpi::log::FileLogger logger{path.string(), *log, "console"}; + + log.reset(); + for (int i = 0; i < 100; ++i) { + { + std::ofstream append{path, std::ios::app}; + append << "line\n"; + } + std::this_thread::sleep_for(std::chrono::milliseconds{10}); + } + + std::filesystem::remove(path); +} + TEST_CASE("FileLoggerTest MissingFileDoesNotDeadlock", "[datalog][file-logger]") { // Constructing a FileLogger for a nonexistent path used to spawn a reader From 1ccb8f5209e9b7ce58098ec71915866734f2529f Mon Sep 17 00:00:00 2001 From: Dustin Spicuzza Date: Sun, 16 Aug 2026 17:06:08 -0400 Subject: [PATCH 2/2] Ignore function --- datalog/src/main/python/semiwrap/DataLog.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/datalog/src/main/python/semiwrap/DataLog.yml b/datalog/src/main/python/semiwrap/DataLog.yml index 712d364d50d..027a6ceda49 100644 --- a/datalog/src/main/python/semiwrap/DataLog.yml +++ b/datalog/src/main/python/semiwrap/DataLog.yml @@ -69,6 +69,8 @@ classes: ignore: true BufferHalfFull: BufferFull: + InvalidateFileLoggerCallbacks: + ignore: true wpi::log::DataLogEntry: force_no_trampoline: true methods: