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

vladstepanyuk · 2025-10-30T09:54:35Z

VERIFY failed (2025-10-30T09:45:33.503210Z): 
  cloud/storage/core/libs/common/thread_pool.cpp:247
  ReleaseWorker(): requirement count > 1 failed
BackTrace(void**, unsigned long)+29 (0x10EB5FD)
FormatBackTrace(IOutputStream*)+32 (0x10EBAD0)
PrintBackTrace()+17 (0x10EBB21)
NPrivate::InternalPanicImpl(int, char const*, char const*, int, int, int, TBasicStringBuf<char, std::__y1::char_traits<char> >, char const*, unsigned long)+995 (0x114C713)
NPrivate::Panic(NPrivate::TStaticBuf const&, int, char const*, char const*, char const*, ...)+418 (0x113F8E2)
??+0 (0x1550736)
??+0 (0x155032C)
??+0 (0x155021D)
??+0 (0x114B216)
??+0 (0x1151AFD)
??+0 (0x7F43337B5609)
clone+67 (0x7F43336DA353)

Сейчас возможен следующий сценарий:
Пусть есть два треда T1, T2 и Thread Pool с одним воркером
изначально до запуска пула RunningWorkers равен 0

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

Line 72 in 155b318

Y_CACHE_ALIGNED TAtomic RunningWorkers = 0;

тред T1 ставит таску на исполнение, увеличивает кол-во RunningWorkers до 1 внутри функции 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(); 
        
               } 
        
           }

тред T1 зависает внутри бесконечного цикла (т.к. нет спящих тредов, которые можно разбудить, из-за того что пулл еще не запущен)

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

Lines 269 to 275 in 155b318

    
           for (;;) { 
        
               for (auto& worker: Workers) { 
        
                   if (WakeUp(worker)) { 
        
                       return; 
        
                   } 
        
               } 
        
           }

тред T2 стартует тред пулл сетит RunningWorkers

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

Lines 98 to 105 in 155b318

    
           void Start() override 
        
           { 
        
               AtomicSet(RunningWorkers, NumWorkers); 
        
               for (auto& worker: Workers) { 
        
                   worker.Thread->Start(); 
        
               } 
        
           }

тред ThreadPool worker успешно исполняет поставленную таску, уменьшает кол-во RunningWorkers до 0, и засыпает на спинлоке в функции Wait на 142 строчке

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

Lines 130 to 145 in 155b318

    
           void Run(TWorker& worker) 
        
           { 
        
               ::NCloud::SetCurrentThreadName(worker.Name); 
        
               NProfiling::TMemoryTagScope tagScope(MemoryTagScope.c_str()); 
        
               while (AtomicGet(ShouldStop) == 0) { 
        
                   if (auto task = Queue.Dequeue()) { 
        
                       task->Execute(); 
        
                       continue; 
        
                   } 
        
                   if (ReleaseWorker()) { 
        
                       Wait(worker); 
        
                   } 
        
               } 
        
           }

тред T1 будит ThreadPool worker'а

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

Line 280 in 155b318

ui32 state = AtomicSwap(&worker.State, TWorker::RUNNING);

тред ThreadPool worker пытается исполнить таску из осереди, вызывает опять функцию ReleaseWorker и крашится на верифайке, т.к. RunningWorkers равен 0

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

Lines 130 to 145 in 155b318

    
           void Run(TWorker& worker) 
        
           { 
        
               ::NCloud::SetCurrentThreadName(worker.Name); 
        
               NProfiling::TMemoryTagScope tagScope(MemoryTagScope.c_str()); 
        
               while (AtomicGet(ShouldStop) == 0) { 
        
                   if (auto task = Queue.Dequeue()) { 
        
                       task->Execute(); 
        
                       continue; 
        
                   } 
        
                   if (ReleaseWorker()) { 
        
                       Wait(worker); 
        
                   } 
        
               } 
        
           }

sharpeye · 2025-10-30T10:30:36Z

В каких условиях сработала верифайка - в каких-то UT?

sharpeye · 2025-10-30T10:27:34Z

cloud/storage/core/libs/common/thread_pool_ut.cpp

+        future.GetValueSync();
+
+        // Sleep to be sure that task will be enqueued before start.
+        Sleep(TDuration::Seconds(1));


А если вызвать 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();

я хотел написать тест который будет крашиться в отсутствии фикса.
Если фикса нет, то 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();

}

}

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

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

vladstepanyuk · 2025-10-30T10:39:33Z

В каких условиях сработала верифайка - в каких-то UT?

Написал интеграционные тесты для фичи с открытием закрытием девайсов в диск агенте ( код тот же что и в пре #4299)
Конкретно крашился диск агент (точнее Нбс с диск агент актором)

yegorskii · 2025-10-30T14:21:49Z

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

        , SpinCycles(DurationToCyclesSafe(SPIN_TIMEOUT))
        , MemoryTagScope(std::move(memoryTagScope))
        , Workers(numWorkers)
+        , RunningWorkers(NumWorkers)


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

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

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

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

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

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

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

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 стартует самым последним почти хоть и передается в другие компоненты которые стартуют раньше.

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

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

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

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

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

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

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

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

github-actions · 2025-10-30T15:40:47Z

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit dac2122.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
9850	9848	0	1	0	1	0

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit dac2122.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
2	2	0	0	0	0	0

github-actions · 2025-10-31T09:32:20Z

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit e4faf1a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
9850	9848	0	1	0	1	0

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit e4faf1a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
2	2	0	0	0	0	0

github-actions · 2025-10-31T14:10:04Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-release-msan: all tests PASSED for commit e4faf1a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7711	7711	0	0	0	0	0

github-actions · 2025-10-31T14:11:56Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-release-asan: all tests PASSED for commit e4faf1a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7729	7729	0	0	0	0	0

github-actions · 2025-10-31T14:13:53Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-release-ubsan: all tests PASSED for commit e4faf1a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7763	7763	0	0	0	0	0

github-actions · 2025-10-31T14:21:02Z

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-release-tsan: some tests FAILED for commit e4faf1a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7717	7709	0	4	0	4	0

github-actions · 2025-10-31T16:11:27Z

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit e4faf1a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
9850	9847	0	2	0	1	0

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit e4faf1a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
4	4	0	0	0	0	0

github-actions · 2025-11-05T06:19:58Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-release-msan: all tests PASSED for commit 1168b9a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7711	7711	0	0	0	0	0

github-actions · 2025-11-05T06:28:31Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-release-asan: all tests PASSED for commit 1168b9a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7729	7729	0	0	0	0	0

github-actions · 2025-11-05T06:28:50Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-release-ubsan: all tests PASSED for commit 1168b9a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7763	7763	0	0	0	0	0

github-actions · 2025-11-05T06:39:09Z

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-release-tsan: some tests FAILED for commit 1168b9a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
7717	7707	0	6	0	4	0

github-actions · 2025-11-05T08:27:40Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 1168b9a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
9840	9838	0	0	0	1	1

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 1168b9a.

TESTS	PASSED	ERRORS	FAILED	FAILED BUILD	SKIPPED	MUTED^?
114	114	0	0	0	0	0

komarevtsev-d · 2025-11-06T05:12:52Z

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


    void Enqueue(ITaskPtr task) override
    {
+        Y_ABORT_UNLESS(AtomicGet(ShouldStop) == 0);


Почему не DEBUG? Зачем взрываться в проде на этом?

fixed crash in thread pool and added test

08b671e

sharpeye reviewed Oct 30, 2025

View reviewed changes

vladstepanyuk requested a review from sharpeye October 30, 2025 10:49

komarevtsev-d previously approved these changes Oct 30, 2025

View reviewed changes

correct issue

dac2122

vladstepanyuk dismissed komarevtsev-d’s stale review via dac2122 October 30, 2025 11:54

vladstepanyuk requested a review from komarevtsev-d October 30, 2025 11:55

SvartMetal self-requested a review October 30, 2025 13:10

yegorskii reviewed Oct 30, 2025

View reviewed changes

vladstepanyuk requested a review from yegorskii October 30, 2025 15:28

replaced future promise with std::latch

e4faf1a

added verify on thread pool stopped

1168b9a

komarevtsev-d reviewed Nov 6, 2025

View reviewed changes

	void Enqueue(ITaskPtr task) override
	{
	Queue.Enqueue(std::move(task));

	if (AllocateWorker()) {
	WakeUpWorker();
	}
	}

	for (;;) {
	for (auto& worker: Workers) {
	if (WakeUp(worker)) {
	return;
	}
	}
	}

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

	for (auto& worker: Workers) {
	worker.Thread->Start();
	}
	}

	void Run(TWorker& worker)
	{
	::NCloud::SetCurrentThreadName(worker.Name);
	NProfiling::TMemoryTagScope tagScope(MemoryTagScope.c_str());

	while (AtomicGet(ShouldStop) == 0) {
	if (auto task = Queue.Dequeue()) {
	task->Execute();
	continue;
	}

	if (ReleaseWorker()) {
	Wait(worker);
	}
	}
	}

	// 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);

	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);

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

vladstepanyuk commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharpeye commented Oct 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladstepanyuk commented Oct 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SvartMetal Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 31, 2025

Uh oh!

github-actions bot commented Oct 31, 2025

Uh oh!

github-actions bot commented Oct 31, 2025

Uh oh!

github-actions bot commented Oct 31, 2025

Uh oh!

github-actions bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

github-actions bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vladstepanyuk commented Oct 30, 2025 •

edited

Loading

SvartMetal Nov 6, 2025 •

edited

Loading

github-actions bot commented Oct 30, 2025 •

edited

Loading

github-actions bot commented Oct 31, 2025 •

edited

Loading

github-actions bot commented Oct 31, 2025 •

edited

Loading

github-actions bot commented Nov 5, 2025 •

edited

Loading