zanmato1984 commented on code in PR #49860:
URL: https://github.com/apache/arrow/pull/49860#discussion_r3144826428
##########
cpp/src/arrow/util/async_util.cc:
##########
@@ -466,7 +466,14 @@ Future<>
AsyncTaskScheduler::Make(FnOnce<Status(AsyncTaskScheduler*)> initial_ta
auto scope = START_SCOPED_SPAN_SV(span, "AsyncTaskScheduler::InitialTask"sv);
auto scheduler =
std::make_unique<AsyncTaskSchedulerImpl>(std::move(stop_token),
std::move(abort_callback));
- Status initial_task_st = std::move(initial_task)(scheduler.get());
+ Status initial_task_st;
+ try {
+ initial_task_st = std::move(initial_task)(scheduler.get());
+ } catch (const std::exception& e) {
Review Comment:
Nice fix for GH-47642 — wrapping the `initial_task` invocation ensures
`OnTaskFinished()` is always called so the scheduler can’t hang indefinitely.
Minor IWYU: this now uses `std::exception` but doesn’t include `<exception>`
explicitly.
##########
cpp/src/arrow/util/async_util_test.cc:
##########
@@ -204,6 +205,31 @@ TEST(AsyncTaskScheduler, InitialTaskFails) {
ASSERT_FINISHES_AND_RAISES(Invalid, finished);
}
+TEST(AsyncTaskScheduler, InitialTaskThrowsException) {
+ // If the initial task throws a C++ exception (not a Status), the scheduler
+ // should catch it, convert to a failed Status, and not hang indefinitely.
+ // See https://github.com/apache/arrow/issues/47642
+
+ // Case 1: initial task throws with no other tasks
+ Future<> finished =
+ AsyncTaskScheduler::Make([&](AsyncTaskScheduler* scheduler) -> Status {
+ throw std::runtime_error("some exception");
Review Comment:
The new test coverage looks good.
Two small follow-ups:
- IWYU: this uses `std::runtime_error` — please consider adding
`<stdexcept>` explicitly.
- Since production code also has a `catch (...)` branch, it may be worth
adding a tiny sub-case that throws a non-`std::exception` (e.g. `throw 42;`) to
exercise the “unknown exception” path and assert the message.
--
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]
