Copilot commented on code in PR #51107:
URL: https://github.com/apache/arrow/pull/51107#discussion_r3918549659
##########
cpp/src/arrow/util/thread_pool_test.cc:
##########
@@ -832,6 +833,36 @@ TEST_F(TestThreadPool, SetCapacity) {
ASSERT_EQ(pool->GetCapacity(), 7);
}
#endif
+
+#if defined(ARROW_ENABLE_THREADING) && !defined(_WIN32)
+TEST_F(TestThreadPool, FailedWorkerLaunch) {
+# ifdef __APPLE__
+ GTEST_SKIP() << "RLIMIT_NPROC does not limit thread creation on macOS";
+# else
+ auto pool = this->MakeThreadPool(4);
+
+ struct rlimit limit;
+ ASSERT_EQ(getrlimit(RLIMIT_NPROC, &limit), 0);
+ const rlim_t soft_limit = limit.rlim_cur;
+ limit.rlim_cur = 1;
+ if (setrlimit(RLIMIT_NPROC, &limit) != 0) {
Review Comment:
This test temporarily lowers `RLIMIT_NPROC` in the main test runner process.
Since it’s a process-wide limit, it can inadvertently affect unrelated thread
creation (including in other tests or background threads) and become flaky. The
PR description mentions doing this in a forked child; following the existing
`TestThreadPoolForkSafety` pattern (fork, setrlimit+spawn+assert in child, exit
code checked by parent) would better isolate the side effects.
##########
cpp/src/arrow/util/thread_pool.cc:
##########
@@ -692,17 +693,23 @@ static void SetCurrentThreadPool(ThreadPool* pool) {
current_thread_pool_ = pool
bool ThreadPool::OwnsThisThread() { return GetCurrentThreadPool() == this; }
-void ThreadPool::LaunchWorkersUnlocked(int threads) {
+Status ThreadPool::LaunchWorkersUnlocked(int threads) {
std::shared_ptr<State> state = sp_state_;
for (int i = 0; i < threads; i++) {
state_->workers_.emplace_back();
auto it = --(state_->workers_.end());
- *it = std::thread([this, state, it] {
- SetCurrentThreadPool(this);
- WorkerLoop(state, it);
- });
+ try {
+ *it = std::thread([this, state, it] {
+ SetCurrentThreadPool(this);
+ WorkerLoop(state, it);
+ });
+ } catch (const std::exception& e) {
+ state_->workers_.erase(it);
+ return Status::UnknownError("Failed to launch worker thread: ",
e.what());
+ }
Review Comment:
`LaunchWorkersUnlocked` now catches the `std::thread` construction failure
and converts it to `Status::UnknownError`, which changes behavior from the
documented/PR-described propagation of `std::system_error` (and makes the
failure non-exceptional to callers). If the intent is to preserve the existing
exception behavior while fixing state corruption, consider erasing the
`workers_` entry and rethrowing the original `std::system_error` after cleanup;
otherwise the PR description/tests should be updated to reflect the user-facing
behavior change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
