From 713ffb3bda09e4f72f17c5029d8dd7e73dd1d2a5 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Mon, 8 May 2023 13:22:31 +0000 Subject: [PATCH 1/2] fix race in Context::createCopy --- src/Interpreters/Context.cpp | 4 +- src/Interpreters/tests/gtest_context_race.cpp | 49 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 src/Interpreters/tests/gtest_context_race.cpp diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 31c0f2fc87a..ee2fc0c5c4e 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -669,13 +669,15 @@ SharedContextHolder Context::createShared() ContextMutablePtr Context::createCopy(const ContextPtr & other) { + auto lock = other->getLock(); return std::shared_ptr(new Context(*other)); } ContextMutablePtr Context::createCopy(const ContextWeakPtr & other) { auto ptr = other.lock(); - if (!ptr) throw Exception(ErrorCodes::LOGICAL_ERROR, "Can't copy an expired context"); + if (!ptr) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Can't copy an expired context"); return createCopy(ptr); } diff --git a/src/Interpreters/tests/gtest_context_race.cpp b/src/Interpreters/tests/gtest_context_race.cpp new file mode 100644 index 00000000000..32922305435 --- /dev/null +++ b/src/Interpreters/tests/gtest_context_race.cpp @@ -0,0 +1,49 @@ +#include +#include +#include + +using namespace DB; + +template +void run(Ptr && context) +{ + for (size_t i = 0; i < 100; ++i) + { + std::thread t1([context] + { + if constexpr (std::is_same_v) + context.lock()->getAsyncReadCounters(); + else + context->getAsyncReadCounters(); + }); + + std::thread t2([context] + { + Context::createCopy(context); + }); + + t1.join(); + t2.join(); + } +} + +TEST(Context, MutableRace) +{ + auto context = Context::createCopy(getContext().context); + context->makeQueryContext(); + run(std::move(context)); +} + +TEST(Context, ConstRace) +{ + auto context = Context::createCopy(getContext().context); + context->makeQueryContext(); + run(std::move(context)); +} + +TEST(Context, WeakRace) +{ + auto context = Context::createCopy(getContext().context); + context->makeQueryContext(); + run(std::move(context)); +} From d88224dc43694a87466da6ff7c3e4a81e51d8c09 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Tue, 9 May 2023 13:59:06 +0000 Subject: [PATCH 2/2] fix build --- src/Interpreters/tests/gtest_context_race.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Interpreters/tests/gtest_context_race.cpp b/src/Interpreters/tests/gtest_context_race.cpp index 32922305435..60531494592 100644 --- a/src/Interpreters/tests/gtest_context_race.cpp +++ b/src/Interpreters/tests/gtest_context_race.cpp @@ -5,7 +5,7 @@ using namespace DB; template -void run(Ptr && context) +void run(Ptr context) { for (size_t i = 0; i < 100; ++i) { @@ -31,19 +31,19 @@ TEST(Context, MutableRace) { auto context = Context::createCopy(getContext().context); context->makeQueryContext(); - run(std::move(context)); + run(context); } TEST(Context, ConstRace) { auto context = Context::createCopy(getContext().context); context->makeQueryContext(); - run(std::move(context)); + run(context); } TEST(Context, WeakRace) { auto context = Context::createCopy(getContext().context); context->makeQueryContext(); - run(std::move(context)); + run(context); }