westonpace commented on a change in pull request #9644:
URL: https://github.com/apache/arrow/pull/9644#discussion_r601451364
##########
File path: cpp/src/arrow/util/thread_pool.h
##########
@@ -104,15 +104,22 @@ class ARROW_EXPORT Executor {
template <typename T>
Future<T> Transfer(Future<T> future) {
auto transferred = Future<T>::Make();
- future.AddCallback([this, transferred](const Result<T>& result) mutable {
+ auto callback = [this, transferred](const Result<T>& result) mutable {
auto spawn_status = Spawn([transferred, result]() mutable {
transferred.MarkFinished(std::move(result));
});
if (!spawn_status.ok()) {
transferred.MarkFinished(spawn_status);
}
- });
- return transferred;
+ };
+ auto callback_factory = [&callback]() { return callback; };
+ if (future.TryAddCallback(callback_factory)) {
+ return transferred;
+ }
+ // If the future is already finished and we aren't going to force spawn a
thread
+ // then we don't need to add another layer of callback and can return the
original
+ // future
Review comment:
In this case the future is already finished. There is nothing we can
do. The old implementation suffered from the same problem.
`Transfer(finished_fut).AddCallback(...)` will run the callback synchronously
on the calling thread regardless of what you do inside of
`Transfer(finished_fut)`.
So far this isn't a problem, the only place we call transfer is from the CPU
thread pool to take something off the I/O thread pool. If the future is
finished then the callback will run synchronously on the CPU thread pool
anyways. The only place it could really be an issue I suppose is if you were
trying to transfer it onto a different thread pool than the calling thread pool.
That being said I could revert all the changes to this method. They were
leftover from an earlier misunderstanding and they don't change the behavior of
`Transfer` at all. I simply left it this way because I felt it was clearer
about the effects.
--
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
