westonpace commented on a change in pull request #9095:
URL: https://github.com/apache/arrow/pull/9095#discussion_r565969245
##########
File path: cpp/src/arrow/util/task_group.cc
##########
@@ -135,6 +149,18 @@ class ThreadedTaskGroup : public TaskGroup {
// before cv.notify_one() has returned
std::unique_lock<std::mutex> lock(mutex_);
cv_.notify_one();
+ if (completion_future_.has_value()) {
+ // MarkFinished could be slow. We don't want to call it while we are
holding
+ // the lock.
+ // TODO: If optional is thread safe then we can skip this locking
entirely
+ auto future = *completion_future_;
+ auto finished = completion_future_->is_finished();
+ auto status = status_;
+ lock.unlock();
+ if (!finished) {
+ future.MarkFinished(status);
Review comment:
I struggled to create a test case that could reproduce this. It seems
like it should be reproducible but even adding a slow callback (so MarkFinished
took a long time) it was difficult to get a task to be added and then finish
just when I wanted it to. I have added some logic however so that this race
condition should not be possible any longer.
##########
File path: cpp/src/arrow/util/thread_pool.h
##########
@@ -87,6 +87,21 @@ class ARROW_EXPORT Executor {
return SpawnReal(hints, std::forward<Function>(func));
}
+ template <typename T>
+ Future<T> Transfer(Future<T> future) {
+ auto transferred = Future<T>::Make();
+ future.AddCallback([this, transferred](const Result<T>& result) mutable {
+ Result<T> result_copy(result);
Review comment:
This copy is removed completely.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
