zanmato1984 commented on PR #49860: URL: https://github.com/apache/arrow/pull/49860#issuecomment-4324036900
Thanks for addressing GH-47642 — wrapping the `initial_task` invocation in `AsyncTaskScheduler::Make()` with `try`/`catch` ensures `OnTaskFinished()` is always called so `running_tasks_` can reach zero and the scheduler’s `finished` future can’t hang indefinitely. The new unit test also looks good: it covers both “throws with no other tasks” and “throws after scheduling a task”, and asserts the `finished` future stays pending until the outstanding task completes. A couple small follow-ups: - IWYU: `cpp/src/arrow/util/async_util.cc` now uses `std::exception` — please consider adding `#include <exception>`; the test uses `std::runtime_error` — please consider adding `#include <stdexcept>`. - Since production code also has a `catch (...)` branch, would you mind adding a tiny sub-case that throws a non-`std::exception` type (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]
