Skip to content

fixed crash in the thread pool occurs because of enqueueing tasks before the thread pool starts #4583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+36 −2
Open

fixed crash in the thread pool occurs because of enqueueing tasks before the thread pool starts #4583

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
08b671e
fixed crash in thread pool and added test
vladstepanyuk Oct 30, 2025
dac2122
correct issue
vladstepanyuk Oct 30, 2025
e4faf1a
replaced future promise with std::latch
vladstepanyuk Oct 31, 2025
1168b9a
added verify on thread pool stopped
vladstepanyuk Nov 5, 2025
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
3 changes: 1 addition & 2 deletions cloud/storage/core/libs/common/thread_pool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class TThreadPool final
, SpinCycles(DurationToCyclesSafe(SPIN_TIMEOUT))
, MemoryTagScope(std::move(memoryTagScope))
, Workers(numWorkers)
, RunningWorkers(NumWorkers)
Copy link
Collaborator

@yegorskii yegorskii Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кажется теперь название не соответствует действительности. В конструкторе воркеры не запускаются. Ну и да, очень подозрительно выглядит сценарий когда мы начинаем юзать пул без вызова Start (у нас тогда любой IStartable по идее может страдать от такого же). Если кто-то так делает, то лучше наверное эту проблему решать в Execute (бросать исключение, ждать на фьюче до вызова старт), но в общем попытка использования threadpool без вызова Start выглядит как нарушение "контракта"

Copy link
Collaborator Author

@vladstepanyuk vladstepanyuk Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не понимаю, почему это нарушение контракта. Тот же шедулер (https://github.com/ydb-platform/nbs/blob/6cfc50c76085138d8908ffa4bdef8bb3ecb22f4a/cloud/blockstore/libs/daemon/common/bootstrap.cpp#L966C28-L966C37) стартует самым последним, и любая компонента может зашедулить таски в него до его старта, тред пул — очень похожая по смыслу штука. Тем более у нас в компонентах циклы по зависимостям, и как бэ наоборот надо стремиться к тому, чтобы все компоненты корректно себя вели, если их ручки дергают до старта, а не крашить процесс исключением, если такое происходит. Да и в целом есть 2 однострочных фикса (одинаковых по сложности): один из них расширяет допустимые сценарии использования объекта, убирает возможность случайно совершить ошибку, а второй опирается на какие-то совершенно неочевидные контракты(которые к одним компонентам приложимы, к другим нет) и фиксит только поведение внутри моего пра, при этом кто-то через N времени может заиспользовать случайно тред пул до его старта и получить исключение и, соответственно, краш. И ладно, если это произойдет при тестировании, такое может и до прода успеть доехать и крашнуть процесс там, я уже фиксил баги, которые стреляют только в проде, если какую-то из ручек дернули слишком рано на старте процесса, так что такой сценарий мне кажется довольно вероятным

Copy link
Collaborator Author

@vladstepanyuk vladstepanyuk Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну а насчет названия, оно и до этого не сильно соответствовало реальности. Тип этот атомик означает кол-во воркеров, которые не ждут задачки в функции wait, по сути. Можно, конечно, переименовать, но тогда уж лучше отдельным пром наверное, или изменить RunningWorkers на WaitingWorkers

Copy link
Collaborator

@SvartMetal SvartMetal Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Откуда берётся сценарий использования трэд пула до его старта?
Такого не должно происходит, если происходит - нужно чинить вызывающий код

Плюс в Enqueue должна быть верифайка что thread pool в состоянии started

Copy link
Collaborator Author

@vladstepanyuk vladstepanyuk Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Откуда берётся сценарий использования трэд пула до его старта?
Такого не должно происходит, если происходит - нужно чинить вызывающий код

Почему? У нас многие компоненты используются до старта. Тот же шедулер, который я уже упомянул.
Там прям целый отдельный комент что так задумано и что у нас есть циклы в зависимостях, а если есть циклы то есть и теоритическая вероятность, что кто-то что-то заиспользует до старта.

nbs/cloud/blockstore/libs/daemon/common/bootstrap.cpp

Lines 961 to 966 in e4faf1a

// we need to start scheduler after all other components for 2 reasons:
// 1) any component can schedule a task that uses a dependency that hasn't
// started yet
// 2) we have loops in our dependencies, so there is no 'correct' starting
// order
START_COMMON_COMPONENT(Scheduler);

помимо шедулера в акторную систему передаются RdmaClient, EndpointManager(через EndpointEventHandler)

nbs/cloud/blockstore/libs/daemon/common/bootstrap.cpp

Lines 945 to 959 in e4faf1a

START_KIKIMR_COMPONENT(ActorSystem);
START_COMMON_COMPONENT(EndpointManager);
START_COMMON_COMPONENT(Service);
START_COMMON_COMPONENT(VhostServer);
START_COMMON_COMPONENT(NbdServer);
START_COMMON_COMPONENT(GrpcEndpointListener);
START_COMMON_COMPONENT(Executor);
START_COMMON_COMPONENT(Server);
START_COMMON_COMPONENT(ServerStatsUpdater);
START_COMMON_COMPONENT(BackgroundThreadPool);
START_COMMON_COMPONENT(RdmaClient);
START_COMMON_COMPONENT(GetTraceServiceClient());
START_COMMON_COMPONENT(RdmaRequestServer);
START_COMMON_COMPONENT(RdmaTarget);
START_COMMON_COMPONENT(CellManager);

которые стартуют позже акторной системы

CellManager стартует самым последним почти хоть и передается в другие компоненты которые стартуют раньше.

Особенно вот вообще неочевидно почему для шедулера это допустимо и нормально, а для тред пула это недопустимое поведение.

Тем более сделать так чтобы можно было использовать тред пул до старта легко, буквально перетащить одну строчку, так зачем добавлять какую-то верифайку, на которую можно наткнуться максимально неожиданным способом причем в проде.

Copy link
Collaborator

@sharpeye sharpeye Nov 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тем более сделать так чтобы можно было использовать тред пул до старта легко, буквально перетащить одну строчку, так зачем добавлять какую-то верифайку, на которую можно наткнуться максимально неожиданным способом причем в проде.

А что с остановкой? Если тредпул используется после остановки - это явная ошибка.

Copy link
Collaborator

@komarevtsev-d komarevtsev-d Nov 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А что с остановкой? Если тредпул используется после остановки - это явная ошибка.

Я тут в целом согласен, что это может привести к неожиданным эффектам, из-за того что задача не исполнится. То есть код, который постит что-то в остановленный тред пул, лучше не множить и подсветить разботчику что он делает что-то не то.
Поэтому кажется достаточно Y_DEBUG_ABORT_UNLESS в этом месте. В проде взрываться смысла не вижу, т.к. критичность у таких багов низкая

Copy link
Collaborator

@SvartMetal SvartMetal Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Поговорили голосом. Перемещение RunningWorkers в конструктор, формально, не вносит неконсистентность в код, а даже исправляет её, потому что в конструкторе инициализируются Workers в стейте RUNNING, поэтому логично инициализировать RunningWorkers == Workers.size() == NumWorkers.

При этом остаётся формальная неконсистентность потому что после запуска конструктора воркеры не совсем running, ибо требуется вызов Start чтобы функция воркера начала выполнение. Но это минор

{
size_t i = 1;
for (auto& worker: Workers) {
Expand All @@ -97,8 +98,6 @@ class TThreadPool final

void Start() override
{
AtomicSet(RunningWorkers, NumWorkers);

for (auto& worker: Workers) {
worker.Thread->Start();
}
Expand Down
33 changes: 33 additions & 0 deletions cloud/storage/core/libs/common/thread_pool_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

#include <util/generic/scope.h>

#include <thread>

namespace NCloud {

////////////////////////////////////////////////////////////////////////////////
Expand All @@ -30,6 +32,37 @@ Y_UNIT_TEST_SUITE(TThreadPoolTest)

UNIT_ASSERT_EQUAL(future.GetValue(WaitTimeout), 42);
}

Y_UNIT_TEST(ShouldExecuteTaskEnqueuedBeforeStart)
{
auto threadPool = CreateThreadPool("thread", 1);

auto promise = NThreading::NewPromise();

auto future = promise.GetFuture();

std::thread thread(
[threadPool, promise = std::move(promise)]() mutable
{
promise.SetValue();
auto future = threadPool->Execute([] { return 42; });

UNIT_ASSERT_EQUAL(future.GetValue(WaitTimeout), 42);
});

future.GetValueSync();

// Sleep to be sure that task will be enqueued before start.
Sleep(TDuration::Seconds(1));
Copy link
Collaborator

@sharpeye sharpeye Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А если вызвать promise.SetValue(); после threadPool->Execute ?

Можно взять latch для синхронизации:

auto threadPool = CreateThreadPool("thread", 1);

std::latch enqueued{1};

std::thread thread([&] {
    auto future = threadPool->Execute([] { return 42; });
    enqueued.count_down();

    UNIT_ASSERT_EQUAL(42, future.GetValue(WaitTimeout));
});

enqueued.wait();

threadPool->Start();
threadPool->Stop();

thread.join();

Copy link
Collaborator Author

@vladstepanyuk vladstepanyuk Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

я хотел написать тест который будет крашиться в отсутствии фикса.
Если фикса нет, то threadPool->Execute зависнет пока не вызовется threadPool->Start() т.е. будет дедлок.
Как бы да зафейлится тест но хотелось бы краш
по хорошему надо было бы вставить count_down после AllocateWorker

nbs/cloud/storage/core/libs/common/thread_pool.cpp

Lines 120 to 127 in 155b318

void Enqueue(ITaskPtr task) override
{
Queue.Enqueue(std::move(task));
if (AllocateWorker()) {
WakeUpWorker();
}
}

но так сделать понятно не получится. Так что не понятно как воспроизвести стабильно краш без слипов и без того чтобы лезть в кишки тред пула

Copy link
Collaborator

@sharpeye sharpeye Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

enqueued.wait();
// Sleep to make it more likely that ReleaseWorker is called before starting the thread pool
Sleep(1s);
threadPool->Start();


threadPool->Start();
Y_DEFER
{
threadPool->Stop();
};

thread.join();
}
}

////////////////////////////////////////////////////////////////////////////////
Expand Down
Loading