GH-48137: [C++] Restore ThreadPool state when a worker fails to start - #51107
Open
advitrocks9 wants to merge 1 commit into
Open
GH-48137: [C++] Restore ThreadPool state when a worker fails to start#51107advitrocks9 wants to merge 1 commit into
advitrocks9 wants to merge 1 commit into
Conversation
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes exception-safety holes in Arrow’s C++ ThreadPool worker-launch path so that a failed std::thread construction cannot leave the pool in a wedged state (stale workers_ entries and inflated tasks_queued_or_running_) that would hang Shutdown() / WaitForIdle() and eventually prevent new workers from being started.
Changes:
- Make
LaunchWorkersUnlocked()erase the just-appendedworkers_entry ifstd::threadconstruction throws, then rethrow. - Move
tasks_queued_or_running_increment inSpawnReal()to after a successful worker launch attempt (keeping the launch heuristic equivalent). - Add a fork-based regression test that forces thread creation failure and asserts the pool returns to a clean state and can still shut down.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cpp/src/arrow/util/thread_pool.h | Adds FRIEND_TEST access for the new fork-safety regression test. |
| cpp/src/arrow/util/thread_pool.cc | Restores ThreadPool internal invariants when worker thread creation throws; avoids counter leaks on launch failure. |
| cpp/src/arrow/util/thread_pool_test.cc | Adds FailedWorkerLaunch regression test that validates state restoration after forced thread creation failure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
LaunchWorkersUnlockedappends tostate_->workers_before constructing the thread that ownsthat entry, and only the worker itself erases it. If the
std::threadconstructor throws, theentry stays, so
Shutdownwaits forever onworkers_.empty(). The same throw escapesSpawnRealaftertasks_queued_or_running_has been incremented, which wedgesWaitForIdle,and once stale entries fill
workers_to capacity the pool stops launching workers whileSpawnstill returns OK for tasks nothing will run.What changes are included in this PR?
The thread construction is wrapped so a failure erases the entry and rethrows, and the counter
is incremented only after the launch succeeds.
Spawnalready propagatesstd::system_errorfrom this path. Turning it into a
Statuswould report success for a task with no worker.Are these changes tested?
TestThreadPoolForkSafety.FailedWorkerLaunchforces a thread creation failure in a forked child,then checks the pool is back to zero workers and zero tasks and still shuts down. It fails on
main, and reverting either hunk brings the failure back. The child skips if it cannot make thread
creation fail.
Are there any user-facing changes?
No. A failed spawn throws the same
std::system_errorand the pool stays usable.