SteNicholas opened a new issue, #463: URL: https://github.com/apache/paimon-cpp/issues/463
### Search before asking - [x] I searched in the [issues](https://github.com/apache/paimon-cpp/issues) and found nothing similar. ### Paimon-cpp version main, commit a397d344c554a8f5fb588189189e47acef47e0f0 ### Minimal reproduce step Build with ThreadSanitizer (`-DPAIMON_USE_TSAN=ON`) and register the first `LoggerCreator` while another thread calls `Logger::GetLogger`: ```cpp std::atomic<bool> stop{false}; std::thread getter([&stop]() { while (!stop.load()) { auto logger = paimon::Logger::GetLogger("getter"); } }); paimon::Logger::RegisterLogger( [](const std::string&) -> std::unique_ptr<paimon::Logger> { return nullptr; }); stop.store(true); getter.join(); ``` ### What doesn't meet your expectations? Expected `RegisterLogger` and `GetLogger` to be safe to call concurrently, since both guard the logger creator with `getRegistryLock()`. Instead, `GetLogger` in `src/paimon/common/logging/logging.cpp` reads the creator before it takes the lock: ```cpp void Logger::RegisterLogger(LoggerCreator creator) { std::unique_lock<std::shared_mutex> lock(getRegistryLock()); getLoggerCreator() = creator; // write under exclusive lock } std::unique_ptr<Logger> Logger::GetLogger(const std::string& path) { auto& creator = getLoggerCreator(); if (creator) { // read without lock std::shared_lock<std::shared_mutex> lock(getRegistryLock()); return creator.value()(path); // creator runs under shared lock } std::unique_lock<std::shared_mutex> ulock(getRegistryLock()); // exclusive lock on every call if (!google::IsGoogleLoggingInitialized()) { google::InitGoogleLogging(GetProgramName()); } return std::make_unique<GlogAdaptor>(); } ``` This causes three problems: 1. **Data race on first registration.** The first `RegisterLogger` call changes the `std::optional` from empty to engaged. `if (creator)` reads that state with no synchronization, so the read and the write race. This is undefined behavior under the C++17 memory model, and taking the shared lock afterwards does not make the earlier read safe. Later registrations only replace the stored `std::function` and leave the engaged state unchanged, so the race is on the empty-to-engaged transition. Registering once before any worker thread starts avoids it, but nothing enforces that order. 2. **Creator runs while holding shared lock.** If a creator calls `RegisterLogger`, the same thread requests exclusive ownership of `getRegistryLock()` while it already holds shared ownership. `std::shared_mutex` is not recursive, so this is undefined behavior and may deadlock. 3. **Default path serializes all callers.** Without a registered creator, every `GetLogger` call takes the global exclusive lock, even after glog is initialized, and the lock also covers the `GlogAdaptor` allocation. `GetLogger` is called from constructors such as `SingleFileWriter`, `RollingFileWriter`, `CompactTask` and `KeyValueFileStoreWrite`, so threads that create these objects concurrently queue on one mutex. The performance impact has not been measured. Existing tests do not cover the race: `TestMultiThreadGetLogger` only calls `GetLogger` concurrently, and `TestRegisterCustomLoggerCreator` registers and gets sequentially in a separate process. ### Anything else? Proposed fix: check and copy the creator under the shared lock, call it after releasing the lock, and initialize glog once with `std::call_once`: ```cpp std::unique_ptr<Logger> Logger::GetLogger(const std::string& path) { std::optional<LoggerCreator> creator; { std::shared_lock<std::shared_mutex> lock(getRegistryLock()); creator = getLoggerCreator(); } if (creator) { return (*creator)(path); } static std::once_flag init_flag; std::call_once(init_flag, [] { if (!google::IsGoogleLoggingInitialized()) { google::InitGoogleLogging(GetProgramName()); } }); return std::make_unique<GlogAdaptor>(); } ``` Add a regression test that registers the first creator while another thread calls `GetLogger`. Registration cannot be undone, so run it in a separate process like `TestRegisterCustomLoggerCreator`, and make sure a ThreadSanitizer report fails the test in the TSan CI jobs. ### Are you willing to submit a PR? - [x] I'm willing to submit a PR! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
