SteNicholas commented on code in PR #203:
URL: https://github.com/apache/paimon-cpp/pull/203#discussion_r3801691270
##########
src/paimon/common/factories/io_hook_test.cpp:
##########
@@ -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]() {
Review Comment:
The reset thread starts before the worker threads are even created, so it
can finish all iterations before any `Try()` runs. Because every iteration ends
with `Clear()`, the test then passes in the final `SILENT` state without
exercising concurrent `Reset()`/`Try()`. Please add a shared start barrier and
keep the reset loop active until the workers signal completion so overlap is
guaranteed.
##########
src/paimon/common/factories/singleton_test.cpp:
##########
@@ -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) {
Review Comment:
This regression gate depends on this test being the first code to
instantiate `Singleton<IOHook>`, but CMake source order does not guarantee
cross-translation-unit test registration/execution order across linkers, and
`--gtest_shuffle` can reorder it. If another test calls `GetInstance()` first,
this storm only exercises the already-published fast path and cannot detect the
original publication race. Please make the first construction test-controlled,
for example with a dedicated test singleton or an isolated test executable.
--
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]
