lucasfang commented on code in PR #203:
URL: https://github.com/apache/paimon-cpp/pull/203#discussion_r3818917296
##########
src/paimon/common/factories/io_hook.cpp:
##########
@@ -29,18 +31,19 @@ namespace paimon {
class IOHook::Impl {
public:
Status Try(const std::string& path) {
- if (io_count_.fetch_add(1) < pos_.load()) {
+ std::shared_lock<std::shared_mutex> lock(mutex_);
Review Comment:
The race fix itself is welcome — Reset()/Clear() and Try() were genuinely
racy before. My concern is where the lock landed: Try() is invoked by
CHECK_HOOK on every local-file IO (in local_file.cpp, hook_ is always
initialized to IOHook::GetInstance(), so the guard never skips it). The old
implementation was lock-free (fetch_add + an atomic pos_ load); the new one
takes a std::shared_mutex shared lock on every IO.
Suggested fix: gate the lock behind an "armed" flag. The hook is never armed
in production — the default state is exactly what Clear() sets up (pos_ = -1,
SILENT), and writers only exist in tests. So we can keep the mutex for
correctness on the armed path while restoring a lock-free fast path for the
disabled state:
--
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]
