Skip to content

Commit 157575a

Browse files
authored
GH-48137: [C++] Restore ThreadPool state when a worker fails to start (#51107)
### Rationale for this change `LaunchWorkersUnlocked` appends an entry to `state_->workers_` before constructing the thread that owns it, and only the worker itself erases that entry. If the `std::thread` constructor fails, the entry stays behind with nothing left to remove it, so `Shutdown` waits forever on `workers_.empty()`. The destructor takes the same path. The failure also escaped `SpawnReal` after `tasks_queued_or_running_` had been incremented, so `WaitForIdle` never returned either, and once stale entries filled `workers_` to capacity the pool stopped launching workers while `Spawn` still returned OK for tasks nothing would run. ### What changes are included in this PR? `LaunchWorkersUnlocked` returns a `Status`. A failed thread construction erases the entry it had reserved and returns an error, which `SpawnReal` and `SetCapacity` propagate. The task counter is incremented after the launch rather than before, so a failed launch cannot leak a count. ### Are these changes tested? `TestThreadPool.FailedWorkerLaunch` lowers `RLIMIT_NPROC` to 1, spawns a task, restores the soft limit, and then checks the pool reports no workers and no tasks and still shuts down. It skips on macOS, where `RLIMIT_NPROC` counts processes rather than threads, and skips anywhere else the lowered limit does not stop thread creation, such as under root. ### Are there any user-facing changes? Yes. `Spawn`, `Submit` and `SetCapacity` used to let a `std::system_error` escape when the OS refused a new thread. They return an error `Status` now. `ThreadPool::Make` is unaffected, since worker threads are only started on demand and a new pool starts none. * GitHub Issue: #48137 Authored-by: Advit Arora <advitarora2@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
1 parent 987e231 commit 157575a

3 files changed

Lines changed: 49 additions & 10 deletions

File tree

cpp/src/arrow/util/thread_pool.cc

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include <list>
2424
#include <mutex>
2525
#include <string>
26+
#include <system_error>
2627
#include <thread>
2728
#include <vector>
2829

@@ -580,7 +581,7 @@ Status ThreadPool::SetCapacity(int threads) {
580581
threads - static_cast<int>(state_->workers_.size()));
581582
if (required > 0) {
582583
// Some tasks are pending, spawn the number of needed threads immediately
583-
LaunchWorkersUnlocked(required);
584+
RETURN_NOT_OK(LaunchWorkersUnlocked(required));
584585
} else if (required < 0) {
585586
// Excess threads are running, wake them so that they stop
586587
state_->cv_.notify_all();
@@ -692,17 +693,23 @@ static void SetCurrentThreadPool(ThreadPool* pool) { current_thread_pool_ = pool
692693

693694
bool ThreadPool::OwnsThisThread() { return GetCurrentThreadPool() == this; }
694695

695-
void ThreadPool::LaunchWorkersUnlocked(int threads) {
696+
Status ThreadPool::LaunchWorkersUnlocked(int threads) {
696697
std::shared_ptr<State> state = sp_state_;
697698

698699
for (int i = 0; i < threads; i++) {
699700
state_->workers_.emplace_back();
700701
auto it = --(state_->workers_.end());
701-
*it = std::thread([this, state, it] {
702-
SetCurrentThreadPool(this);
703-
WorkerLoop(state, it);
704-
});
702+
try {
703+
*it = std::thread([this, state, it] {
704+
SetCurrentThreadPool(this);
705+
WorkerLoop(state, it);
706+
});
707+
} catch (const std::exception& e) {
708+
state_->workers_.erase(it);
709+
return Status::UnknownError("Failed to launch worker thread: ", e.what());
710+
}
705711
}
712+
return Status::OK();
706713
}
707714

708715
Status ThreadPool::SpawnReal(TaskHints hints, FnOnce<void()> task, StopToken stop_token,
@@ -729,12 +736,12 @@ Status ThreadPool::SpawnReal(TaskHints hints, FnOnce<void()> task, StopToken sto
729736
return Status::Invalid("operation forbidden during or after shutdown");
730737
}
731738
CollectFinishedWorkersUnlocked();
732-
state_->tasks_queued_or_running_++;
733-
if (static_cast<int>(state_->workers_.size()) < state_->tasks_queued_or_running_ &&
739+
if (static_cast<int>(state_->workers_.size()) <= state_->tasks_queued_or_running_ &&
734740
state_->desired_capacity_ > static_cast<int>(state_->workers_.size())) {
735741
// We can still spin up more workers so spin up a new worker
736-
LaunchWorkersUnlocked(/*threads=*/1);
742+
RETURN_NOT_OK(LaunchWorkersUnlocked(/*threads=*/1));
737743
}
744+
state_->tasks_queued_or_running_++;
738745
state_->pending_tasks_.push(
739746
QueuedTask{{std::move(task), std::move(stop_token), std::move(stop_callback)},
740747
hints.priority,

cpp/src/arrow/util/thread_pool.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -496,6 +496,7 @@ class ARROW_EXPORT ThreadPool : public Executor {
496496

497497
protected:
498498
FRIEND_TEST(TestThreadPool, SetCapacity);
499+
FRIEND_TEST(TestThreadPool, FailedWorkerLaunch);
499500
FRIEND_TEST(TestGlobalThreadPool, Capacity);
500501
ARROW_FRIEND_EXPORT friend ThreadPool* GetCpuThreadPool();
501502

@@ -507,7 +508,7 @@ class ARROW_EXPORT ThreadPool : public Executor {
507508
// Collect finished worker threads, making sure the OS threads have exited
508509
void CollectFinishedWorkersUnlocked();
509510
// Launch a given number of additional workers
510-
void LaunchWorkersUnlocked(int threads);
511+
Status LaunchWorkersUnlocked(int threads);
511512
// Get the current actual capacity
512513
int GetActualCapacity();
513514

cpp/src/arrow/util/thread_pool_test.cc

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717

1818
#ifndef _WIN32
19+
# include <sys/resource.h>
1920
# include <sys/types.h>
2021
# include <unistd.h>
2122
#endif
@@ -832,6 +833,36 @@ TEST_F(TestThreadPool, SetCapacity) {
832833
ASSERT_EQ(pool->GetCapacity(), 7);
833834
}
834835
#endif
836+
837+
#if defined(ARROW_ENABLE_THREADING) && !defined(_WIN32)
838+
TEST_F(TestThreadPool, FailedWorkerLaunch) {
839+
# ifdef __APPLE__
840+
GTEST_SKIP() << "RLIMIT_NPROC does not limit thread creation on macOS";
841+
# else
842+
auto pool = this->MakeThreadPool(4);
843+
844+
struct rlimit limit;
845+
ASSERT_EQ(getrlimit(RLIMIT_NPROC, &limit), 0);
846+
const rlim_t soft_limit = limit.rlim_cur;
847+
limit.rlim_cur = 1;
848+
if (setrlimit(RLIMIT_NPROC, &limit) != 0) {
849+
GTEST_SKIP() << "Could not lower RLIMIT_NPROC";
850+
}
851+
const Status st = pool->Spawn([] {});
852+
limit.rlim_cur = soft_limit;
853+
ASSERT_EQ(setrlimit(RLIMIT_NPROC, &limit), 0);
854+
855+
if (st.ok()) {
856+
GTEST_SKIP() << "Lowering RLIMIT_NPROC did not prevent thread creation";
857+
}
858+
ASSERT_RAISES(UnknownError, st);
859+
ASSERT_EQ(pool->GetActualCapacity(), 0);
860+
ASSERT_EQ(pool->GetNumTasks(), 0);
861+
ASSERT_OK(pool->Shutdown());
862+
# endif
863+
}
864+
#endif
865+
835866
// Test Submit() functionality
836867

837868
TEST_F(TestThreadPool, Submit) {

0 commit comments

Comments
 (0)