Skip to content

Commit d16e3fb

Browse files
tanjialiangmeta-codesync[bot]
authored andcommitted
fix: Fix race condition in MockSharedArbitrationTest.localArbitrationRunInParallelWithGlobalArbitration (#16731)
Summary: Pull Request resolved: #16731 The test spawned `globalArbitrationTriggerThread` before installing the `SCOPED_TESTVALUE_SET` callback for `runGlobalArbitration`. If the thread's allocations triggered global arbitration before the callback was registered, `globalArbitrationStarted` would never be set to true, causing the main thread to deadlock on `globalArbitrationStartWait.await()`. Fix by moving the `SCOPED_TESTVALUE_SET` and its synchronization variables before the thread spawn, ensuring the callback is always registered before any allocation can invoke `runGlobalArbitration`. Fixes #15336 Reviewed By: xiaoxmeng Differential Revision: D96181396 fbshipit-source-id: d4674a551411ddcc1eaf4b82ca5a060d7aeb11be
1 parent 3f02431 commit d16e3fb

File tree

1 file changed

+20
-17
lines changed

1 file changed

+20
-17
lines changed

velox/common/memory/tests/MockSharedArbitratorTest.cpp

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1900,6 +1900,26 @@ DEBUG_ONLY_TEST_F(
19001900
auto* localArbitrationOp = localArbitrationTask->addMemoryOp(true);
19011901
localArbitrationOp->allocate(memoryPoolCapacity);
19021902

1903+
// Install the test value callback BEFORE spawning the thread to avoid a race
1904+
// condition where the thread triggers global arbitration before the callback
1905+
// is registered, causing the main thread to deadlock. (GitHub issue #15336)
1906+
std::atomic_bool globalArbitrationStarted{false};
1907+
folly::EventCount globalArbitrationStartWait;
1908+
std::atomic_bool globalArbitrationWaitFlag{true};
1909+
folly::EventCount globalArbitrationWait;
1910+
SCOPED_TESTVALUE_SET(
1911+
"facebook::velox::memory::SharedArbitrator::runGlobalArbitration",
1912+
std::function<void(const SharedArbitrator*)>(
1913+
([&](const SharedArbitrator* /*unused*/) {
1914+
if (globalArbitrationStarted.exchange(true)) {
1915+
return;
1916+
}
1917+
globalArbitrationStartWait.notifyAll();
1918+
1919+
globalArbitrationWait.await(
1920+
[&]() { return !globalArbitrationWaitFlag.load(); });
1921+
})));
1922+
19031923
auto globalArbitrationTriggerThread = std::thread([&]() {
19041924
std::unordered_map<std::string, RuntimeMetric> runtimeStats;
19051925
auto statsWriter = std::make_unique<TestRuntimeStatWriter>(runtimeStats);
@@ -1952,23 +1972,6 @@ DEBUG_ONLY_TEST_F(
19521972
1);
19531973
});
19541974

1955-
std::atomic_bool globalArbitrationStarted{false};
1956-
folly::EventCount globalArbitrationStartWait;
1957-
std::atomic_bool globalArbitrationWaitFlag{true};
1958-
folly::EventCount globalArbitrationWait;
1959-
SCOPED_TESTVALUE_SET(
1960-
"facebook::velox::memory::SharedArbitrator::runGlobalArbitration",
1961-
std::function<void(const SharedArbitrator*)>(
1962-
([&](const SharedArbitrator* /*unused*/) {
1963-
if (globalArbitrationStarted.exchange(true)) {
1964-
return;
1965-
}
1966-
globalArbitrationStartWait.notifyAll();
1967-
1968-
globalArbitrationWait.await(
1969-
[&]() { return !globalArbitrationWaitFlag.load(); });
1970-
})));
1971-
19721975
globalArbitrationStartWait.await(
19731976
[&]() { return globalArbitrationStarted.load(); });
19741977

0 commit comments

Comments
 (0)