u70b3 commented on code in PR #203: URL: https://github.com/apache/paimon-cpp/pull/203#discussion_r3802025633
########## 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: Good catch — and agreed on the deeper hazard: a regression gate that can pass without exercising the race it guards is worse than no test at all. It does not just miss the bug, it silently certifies the fix (our "TSan-clean" claim rested on this storm), so a vacuous green actively masks the failure it was built to catch. Fixed by making the first construction test-controlled instead of convention-controlled: - `Singleton<T, InstPolicy>::GetInstance()` moved from singleton.cpp into the header, so a test can instantiate `Singleton<T>` for its own type — previously impossible, which is why the storm had to borrow `Singleton<IOHook>` and inherit the ordering fragility. - The storm (renamed `TestConcurrentFirstPublication`) now runs on `FirstPublicationTarget`, a class local to singleton_test.cpp's anonymous namespace. Nothing else in the binary can name the type, so no link order, `--gtest_shuffle`, `--gtest_filter`, or `--gtest_repeat` can pre-publish it: the test is guaranteed to race the first publication by construction. Each thread also verifies full construction visibility (a 64-word payload written by the ctor), which is the essence of the original race. - One subtlety worth recording: the first attempt dropped the explicit instantiations entirely, and local validation immediately caught it — the file-format plugins are separate `-Bsymbolic` shared libraries, so implicit per-library copies of the function-local static state split the singleton (registrations landed in a different instance than lookups, and the wider suites lost the 'orc' factory). `FactoryCreator` and `IOHook` therefore keep extern-template declarations plus explicit instantiations in singleton.cpp; only TU-local test types instantiate implicitly. Validation on Kunpeng-920 (aarch64), latest head (1bc6650): factories suite 9/9 under 20× `--gtest_shuffle` plus a `--gtest_repeat=5 --gtest_shuffle` run; `paimon-common-test` 1436/1436 and `paimon-common-sst-file-format-test` 32/32 (these are the suites that caught the split-singleton issue); TSan clean over 20 shuffled stress runs of the race tests. -- 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]
