westonpace commented on code in PR #15104:
URL: https://github.com/apache/arrow/pull/15104#discussion_r1059422246
##########
cpp/src/arrow/util/async_util.cc:
##########
@@ -188,8 +192,20 @@ class AsyncTaskSchedulerImpl : public AsyncTaskScheduler {
void AbortUnlocked(const Status& st, std::unique_lock<std::mutex>&& lk) {
DCHECK(!st.ok());
+ bool aborted = false;
if (!IsAborted()) {
maybe_error_ = st;
+ // Add one more "task" to represent running the abort callback. This
+ // will prevent any other task finishing and marking the scheduler
finished
+ // while we are running the abort callback.
+ running_tasks_++;
+ aborted = true;
+ }
+ if (aborted) {
+ lk.unlock();
+ std::move(abort_callback_)(st);
+ lk.lock();
+ running_tasks_--;
Review Comment:
Running callbacks while holding locks is problematic. As an example, the
abort callback was added here so we can call StopProducing in the exec plan
when a task fails. If we do this while holding the lock then we have to be
careful, in every node, that we don't use the async task scheduler in any way
during the call to StopProducing. Admittedly, that should probably be obvious
(if we are aborting then scheduling new tasks is meaningless). However, this
is just more overhead for node authors to worry about and I've been trying as
best I can to keep the scheduler fool-proof.
So I run the callback outside of the lock. That was a bit of a pain because
I need to make sure the scheduler doesn't end itself, and get destroyed, while
the abort callback is running. For that reason I treat the abort callback as a
"pseudo-task" by incrementing then decrementing running_tasks_.
--
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]
