egolearner commented on PR #49860: URL: https://github.com/apache/arrow/pull/49860#issuecomment-4461644543
> > If the `initial_task` passed to `AsyncTaskScheduler::Make` throws a C++ exception > > Why would it throw at all? Arrow uses Status-based error reporting. If we were catching exceptions from any user-provided callable, this would litter the Arrow C++ code with ad hoc `catch` statements. We would also certainly forget to catch exceptions in some place, and users would regularly have to report issues about that. > > For reference, our `ThreadPool` doesn't catch exceptions from user tasks either. They are not supposed to throw exceptions. Thanks for the pushback, but I'd like to make a case for keeping the catch here. The failure mode is asymmetric with the rest of Arrow: everywhere else a buggy user callable surfaces as a `Status`, but here it silently hangs forever in release builds (`running_tasks_` stays at 1, the plan's `finished` future never completes). The `ThreadPool` comparison doesn't quite apply either — a throwing task there calls `std::terminate`, so the user gets a stack trace. Here they get silence. Pushing the catch down to call sites (e.g. `SourceNode::StartProducing`) fixes the case I hit but is whack-a-mole — any future caller that hands a throwing callable to `initial_task` will hang again. The centralized fix is one try/catch on the scheduler's bootstrap path, not scattered through the codebase. -- 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]
