westonpace commented on a change in pull request #9528:
URL: https://github.com/apache/arrow/pull/9528#discussion_r578780183
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -515,15 +548,35 @@ class ARROW_MUST_USE_TYPE Future {
}
void DoMarkFinished(Result<ValueType> res) {
- SetResult(std::move(res));
-
- if (ARROW_PREDICT_TRUE(GetResult()->ok())) {
- impl_->MarkFinished();
+ const FutureState state = StateForResult(res);
+ if (impl_->stop_callback_.has_value()) {
+ struct ResultSetter {
+ Future<T>* fut;
+ Result<ValueType> res;
+
+ void operator()() { fut->SetResult(std::move(res)); }
+ };
+ impl_->MarkFinished(state, ResultSetter{this, std::move(res)});
} else {
- impl_->MarkFailed();
+ // The future cannot be cancelled concurrently as CancelOn()
+ // was not called.
+ SetResult(std::move(res));
+ impl_->MarkFinished(state);
}
Review comment:
We already had a lock so if I understand correctly you are pointing out
that the critical section is getting larger (to include the result writing)?
For the waiters it is unlikely to be a big issue. They are waiting already
so they shouldn't be too bothered by a bit more waiting.
For AddCallback/TryAddCallback contention it would have to be a situation
where the future the callback being added to was truly asynchronous (spawned
work in another thread). If it was a purely synchronous future then mark
finished would be finished being called before AddCallback is called. So in
this case the person calling AddCallback has already accepted they might be
adding a callback early and accepting the cost of their task being added to the
scheduler and losing their CPU caches. A little blocking (especially since it
results in them being able to run their callback immediately) should already be
in the range of costs they are willing to accept.
In other words, you already should not be calling AddCallback on a
potentially unfinished future in a performance critical section of code.
----------------------------------------------------------------
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:
us...@infra.apache.org
