Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions include/paimon/factories/singleton.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ class PAIMON_EXPORT LazyInstantiation {
protected:
template <typename T>
static void Create(T*& ptr) {
T* tmp = new T;
MEMORY_BARRIER();
ptr = tmp;
// Publication ordering is handled by the release store in
// Singleton<T, InstPolicy>::GetInstance(), so no barrier is needed here.
ptr = new T;
static std::shared_ptr<T> destroyer(ptr);
}
};
Expand Down
3 changes: 3 additions & 0 deletions src/paimon/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,9 @@ if(PAIMON_BUILD_TESTS)
common/utils/range_helper_test.cpp
common/utils/read_ahead_cache_test.cpp
common/io/cache/lru_cache_test.cpp
common/io/cache/cache_manager_test.cpp
common/utils/byte_range_combiner_test.cpp
common/utils/saturating_cast_test.cpp
common/utils/scope_guard_test.cpp
common/utils/sensitive_config_utils_test.cpp
common/utils/serialization_utils_test.cpp
Expand Down Expand Up @@ -665,6 +667,7 @@ if(PAIMON_BUILD_TESTS)

add_paimon_test(common_factories_test
SOURCES
common/factories/singleton_test.cpp
common/factories/factory_creator_test.cpp
common/factories/io_hook_test.cpp
STATIC_LINK_LIBS
Expand Down
16 changes: 11 additions & 5 deletions src/paimon/common/factories/io_hook.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include "paimon/common/factories/io_hook.h"

#include <atomic>
#include <mutex>
#include <shared_mutex>
#include <stdexcept>

#include "fmt/format.h"
Expand All @@ -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_);
if (io_count_.fetch_add(1) < pos_) {
return Status::OK();
} else {
switch (mode_) {
case IOHook::Mode::SILENT:
return Status::OK();
case IOHook::Mode::RETURN_ERROR:
return Status::IOError(fmt::format(
"io hook triggered io error at position {}, path {}", pos_.load(), path));
"io hook triggered io error at position {}, path {}", pos_, path));
case IOHook::Mode::THROW_EXCEPTION:
throw std::runtime_error(fmt::format(
"io hook throw io exception at position {}, path {}", pos_.load(), path));
"io hook throw io exception at position {}, path {}", pos_, path));
return Status::OK();
default:
return Status::OK();
Expand All @@ -49,12 +52,14 @@ class IOHook::Impl {
}

inline void Reset(int64_t pos, IOHook::Mode mode) {
std::unique_lock<std::shared_mutex> lock(mutex_);
mode_ = mode;
pos_ = pos;
io_count_ = 0;
mode_ = mode;
}

int64_t IOCount() const {
std::shared_lock<std::shared_mutex> lock(mutex_);
return io_count_.load();
}

Expand All @@ -63,8 +68,9 @@ class IOHook::Impl {
}

private:
mutable std::shared_mutex mutex_;
std::atomic<int64_t> io_count_ = {0};
std::atomic<int64_t> pos_ = {-1};
int64_t pos_ = -1;
IOHook::Mode mode_ = IOHook::Mode::SILENT;
};

Expand Down
49 changes: 49 additions & 0 deletions src/paimon/common/factories/io_hook_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,13 @@

#include "paimon/common/factories/io_hook.h"

#include <atomic>
#include <stdexcept>
#include <thread>
#include <vector>

#include "gtest/gtest.h"
#include "paimon/status.h"
#include "paimon/testing/utils/testharness.h"

namespace paimon::test {
Expand Down Expand Up @@ -64,4 +68,49 @@ TEST(IOHookTest, TestThrowExceptionMode) {
hook->Clear();
}

// Regression test for the data race on IOHook's mode: Reset()/Clear() run on one
// thread while other threads call Try() concurrently. Under a ThreadSanitizer build
// this deterministically reports the unsynchronized mode access; functionally it must
// never crash and every Try() must return OK.
TEST(IOHookTest, TestConcurrentResetAndTry) {
auto hook = IOHook::GetInstance();

constexpr int32_t kResetIterations = 200000;
constexpr int32_t kTryIterations = 50000;
constexpr int32_t kNumWorkers = 4;

std::atomic<bool> observed_error{false};

std::thread reset_thread([hook]() {
for (int32_t i = 0; i < kResetIterations; i++) {
hook->Reset(INT64_MAX, IOHook::Mode::RETURN_ERROR);
hook->Clear();
}
});

std::vector<std::thread> workers;
workers.reserve(kNumWorkers);
for (int32_t t = 0; t < kNumWorkers; t++) {
workers.emplace_back([hook, &observed_error]() {
for (int32_t i = 0; i < kTryIterations; i++) {
Status status = hook->Try("concurrent_path");
// Reset() arms an unreachable position, while Clear() uses SILENT mode,
// so both complete states return OK. An IOError exposes a torn state.
if (!status.ok()) {
observed_error.store(true, std::memory_order_relaxed);
}
}
});
}

reset_thread.join();
for (auto& worker : workers) {
worker.join();
}

ASSERT_FALSE(observed_error.load(std::memory_order_relaxed));
// Leave the process-wide singleton in its default SILENT state for later tests.
hook->Clear();
}

} // namespace paimon::test
16 changes: 11 additions & 5 deletions src/paimon/common/factories/singleton.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include "paimon/factories/singleton.h"

#include <atomic>
#include <mutex>

#include "paimon/common/factories/io_hook.h"
Expand All @@ -28,15 +29,20 @@ namespace paimon {

template <typename T, typename InstPolicy>
T* Singleton<T, InstPolicy>::GetInstance() {
static T* ptr;
static std::atomic<T*> ptr{nullptr};
static std::mutex mutex;
if (PAIMON_UNLIKELY(!ptr)) {
T* p = ptr.load(std::memory_order_acquire);
if (PAIMON_UNLIKELY(p == nullptr)) {
std::lock_guard<std::mutex> lg(mutex);
if (!ptr) {
InstPolicy::Create(ptr);
// Re-check under the mutex with a relaxed load; the mutex already
// synchronizes with the creating thread.
p = ptr.load(std::memory_order_relaxed);
if (p == nullptr) {
InstPolicy::Create(p);
ptr.store(p, std::memory_order_release);
}
}
return const_cast<T*>(ptr);
return p;
}

template class Singleton<FactoryCreator>;
Expand Down
88 changes: 88 additions & 0 deletions src/paimon/common/factories/singleton_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

#include "paimon/factories/singleton.h"

#include <array>
#include <atomic>
#include <cstdint>
#include <thread>
#include <vector>

#include "gtest/gtest.h"
#include "paimon/common/factories/io_hook.h"

namespace paimon::test {

namespace {

constexpr int32_t kNumThreads = 32;

// Runs `worker(i)` on kNumThreads threads that are all blocked on a shared start
// flag and released at (nearly) the same time, so that they race on the first
// Singleton::GetInstance() publication. Joins all threads before returning.
template <typename Worker>
void RunStorm(const Worker& worker) {
std::atomic<bool> start{false};
std::vector<std::thread> threads;
threads.reserve(kNumThreads);
for (int32_t i = 0; i < kNumThreads; ++i) {
threads.emplace_back([&start, &worker, i]() {
while (!start.load(std::memory_order_acquire)) {
std::this_thread::yield();
}
worker(i);
});
}
start.store(true, std::memory_order_release);
for (auto& thread : threads) {
thread.join();
}
}

} // namespace

// Regression gate for the Singleton double-checked-locking publication race.
// It only exercises the first construction if nothing has touched
// Singleton<IOHook> before, so this must stay the first GetInstance() call in
// this binary (singleton_test.cpp is the first source of common_factories_test
// and this is its first test). A FactoryCreator storm cannot serve as the
// gate: the REGISTER_PAIMON_FACTORY constructors in paimon_shared already
// initialize Singleton<FactoryCreator> before main().
TEST(SingletonTest, TestConcurrentIOHookGetInstance) {
std::array<IOHook*, kNumThreads> hooks{};
std::array<bool, kNumThreads> try_oks{};
RunStorm([&hooks, &try_oks](int32_t i) {
hooks[i] = Singleton<IOHook>::GetInstance();
// The default (and cleared) IOHook state is SILENT, so Try() must succeed.
try_oks[i] = hooks[i]->Try("singleton_storm_path").ok();
});

IOHook* expected = hooks[0];
ASSERT_NE(expected, nullptr);
for (int32_t i = 0; i < kNumThreads; ++i) {
ASSERT_EQ(expected, hooks[i]);
ASSERT_TRUE(try_oks[i]);
}
ASSERT_GE(expected->IOCount(), kNumThreads);
// Leave the process-wide singleton in its default SILENT state for later tests.
expected->Clear();
}

} // namespace paimon::test
9 changes: 7 additions & 2 deletions src/paimon/common/io/cache/cache_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "paimon/cache/cache.h"
#include "paimon/common/io/cache/cache_key.h"
#include "paimon/common/io/cache/lru_cache.h"
#include "paimon/common/utils/saturating_cast.h"
#include "paimon/memory/memory_segment.h"
#include "paimon/result.h"

Expand Down Expand Up @@ -59,9 +60,13 @@ class PAIMON_EXPORT CacheManager {
/// @param high_priority_pool_ratio Ratio of capacity reserved for index cache [0.0, 1.0).
/// If 0, index and data share the same cache.
CacheManager(int64_t max_memory_bytes, double high_priority_pool_ratio) {
auto index_cache_bytes = static_cast<int64_t>(max_memory_bytes * high_priority_pool_ratio);
// Both factors are config-validated non-negative values, so the products are finite;
// saturation is only a defense against the undefined double->int64_t conversion when
// max_memory_bytes is close enough to INT64_MAX that the product rounds to 2^63.
auto index_cache_bytes =
SaturatingDoubleToInteger<int64_t>(max_memory_bytes * high_priority_pool_ratio);
auto data_cache_bytes =
static_cast<int64_t>(max_memory_bytes * (1.0 - high_priority_pool_ratio));
SaturatingDoubleToInteger<int64_t>(max_memory_bytes * (1.0 - high_priority_pool_ratio));
data_cache_ = std::make_shared<LruCache>(data_cache_bytes);
if (high_priority_pool_ratio == 0.0) {
index_cache_ = data_cache_;
Expand Down
Loading
Loading