You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/29 23:30:40 UTC
[GitHub] [arrow] westonpace opened a new pull request #10205: ARROW-12004: [C++] Result is annoying
westonpace opened a new pull request #10205:
URL: https://github.com/apache/arrow/pull/10205
Per the JIRA
`Future<>::AddCallback` callbacks receive a `Status`.
`Future<T>::AddCallback` callbacks receive a `Result<T>`
`Future<>::Then` callbacks receive nothing
`Future<T>::Then` callbacks receive `const T&`
To achieve this I had to explicitly specialize the empty `Future` but I introduced `FutureBase` to reduce the amount of duplicated code. `detail::Empty` is still around (although it got renamed to `internal::Empty` as a side effect of moving into `functional.h`). It could be removed if one wanted to create a specialized `FutureImpl` but I that doesn't seem to be needed at the moment.
Draft while still in cleanup/WIP but all tests pass on my machine.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r626238894
##########
File path: cpp/src/arrow/util/thread_pool.h
##########
@@ -227,14 +228,14 @@ class ARROW_EXPORT SerialExecutor : public Executor {
std::shared_ptr<State> state_;
template <typename T>
- Result<T> Run(TopLevelTask<T> initial_task) {
+ Future<T> Run(TopLevelTask<T> initial_task) {
auto final_fut = std::move(initial_task)(this);
if (final_fut.is_finished()) {
- return final_fut.result();
+ return final_fut;
}
- final_fut.AddCallback([this](const Result<T>&) { MarkFinished(); });
+ final_fut.AddCallback([this](...) { MarkFinished(); });
Review comment:
Also, I restored the original version.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r626239342
##########
File path: cpp/src/arrow/util/thread_pool_test.cc
##########
@@ -189,11 +189,7 @@ TEST_P(TestRunSynchronously, SpawnMoreNested) {
auto top_level_task = [&](Executor* executor) -> Future<> {
auto fut_a = DeferNotOk(executor->Submit([&] { nested_ran++; }));
auto fut_b = DeferNotOk(executor->Submit([&] { nested_ran++; }));
- return AllComplete({fut_a, fut_b})
- .Then([&](const Result<arrow::detail::Empty>& result) {
- nested_ran++;
- return result;
- });
+ return AllComplete({fut_a, fut_b}).Then([&]() { nested_ran++; });
Review comment:
Thanks, this is great. `call_traits` doesn't work for function pointers so that caused one call to be slightly less readable (there was already a TODO but I promoted it to a full JIRA issue ARROW-12655.
However, I think it's worth it. This caught several places we were accepting `Result` and one in `async_generator.h` that was potentially a bug.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#issuecomment-839428127
> Overall I'm a bit surprised by the amount of complication that seems necessary. Is there a way to straighten this up.
Most of your comments should be addressable. However, I'm not certain if this comment is referring to the overall idea of splitting `Future` into three classes (well two with one specialization) `FutureBase`, `Future<>`, and `Future<T>`.
The need there stems from the fact that I cannot (to my knowledge) apply SFINAE to a method based on a class template parameter (`T`). I think I could take the "dummy parameter" approach (e.g. something like `void f(T&& x, typename std::enable_if<!std::is_reference<T_>::value, std::nullptr_t>::type = nullptr)`) but I personally find that to be less readable and, since the inheritance is non-virtual, I don't think it would be more performant.
Another approach could be to allow `Result<Status>` (perhaps with a special factory method to allow the static assert to remain) but I seem to recall that being frowned upon. If that is on the table then I'd be happy to investigate that option as well.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#issuecomment-840876262
Ok, @bkietz 's tricks **almost** worked. Turns out some older MSVC compilers can get confused when figuring out overloads based only on the return value (https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/39152166/job/ewlnjonukpbaiaet)...
```
..\src\arrow/util/future.h(603): note: could be 'std::enable_if<false,ForReturnImpl<Return>::type>::type arrow::Future<std::shared_ptr<arrow::RecordBatch>>::Then<_Ty,arrow::Future<std::shared_ptr<arrow::RecordBatch>>::Then::<lambda_0aa43ac6ba74e671a09c4f431b6e6fbf>,ForReturnImp
```
^-- MSVC is considering the overload even though the return type is `std::enable_if<false,ForReturnImpl<Return>::type>::type`
So I went ahead and just made a dummy method arg which appears to have satisfied Windows (MinGW failures are unrelated I believe).
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#issuecomment-840670844
I've cleaned up now and, assuming CI passes, this is ready for another round of review. Thanks for all the feedback so far.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r626238841
##########
File path: cpp/src/arrow/util/thread_pool.h
##########
@@ -227,14 +228,14 @@ class ARROW_EXPORT SerialExecutor : public Executor {
std::shared_ptr<State> state_;
template <typename T>
- Result<T> Run(TopLevelTask<T> initial_task) {
+ Future<T> Run(TopLevelTask<T> initial_task) {
auto final_fut = std::move(initial_task)(this);
if (final_fut.is_finished()) {
- return final_fut.result();
+ return final_fut;
}
- final_fut.AddCallback([this](const Result<T>&) { MarkFinished(); });
+ final_fut.AddCallback([this](...) { MarkFinished(); });
Review comment:
What's the source for this rule. I had remembered some rule like this but I couldn't find it when I searched for it so I started to think I had made it up. For example, https://en.cppreference.com/w/cpp/language/parameter_pack doesn't have the word "trivial" anywhere.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r631578198
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -56,6 +47,47 @@ struct is_future<Future<T>> : std::true_type {};
template <typename Signature>
using result_of_t = typename std::result_of<Signature>::type;
+template <typename Source, typename Dest>
+typename std::enable_if<Source::is_empty>::type Propagate(Source* source, Dest dest) {
+ struct MarkNextFinished {
+ void operator()(const Status& status) && { next.MarkFinished(status); }
+ Dest next;
+ };
+ source->AddCallback(MarkNextFinished{std::move(dest)});
+}
+
+template <typename Source, typename Dest, bool SourceEmpty = Source::is_empty,
+ bool DestEmpty = Dest::is_empty>
+struct MarkNextFinished {};
+
+template <typename Source, typename Dest>
+struct MarkNextFinished<Source, Dest, true, false> {
+ void operator()(const Status& status) && { next.MarkFinished(status); }
Review comment:
Correct. I've removed this method.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r631925048
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -78,28 +110,32 @@ struct ContinueFuture {
template <typename ContinueFunc, typename... Args,
typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
typename NextFuture = ForReturn<ContinueResult>>
- typename std::enable_if<!std::is_void<ContinueResult>::value &&
- !is_future<ContinueResult>::value>::type
+ typename std::enable_if<
+ !std::is_void<ContinueResult>::value && !is_future<ContinueResult>::value &&
+ (!NextFuture::is_empty || std::is_same<ContinueResult, Status>::value)>::type
operator()(NextFuture next, ContinueFunc&& f, Args&&... a) const {
next.MarkFinished(std::forward<ContinueFunc>(f)(std::forward<Args>(a)...));
}
+ template <typename ContinueFunc, typename... Args,
+ typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
+ typename NextFuture = ForReturn<ContinueResult>>
+ typename std::enable_if<!std::is_void<ContinueResult>::value &&
+ !is_future<ContinueResult>::value && NextFuture::is_empty &&
+ !std::is_same<ContinueResult, Status>::value>::type
+ operator()(NextFuture next, ContinueFunc&& f, Args&&... a) const {
+ next.MarkFinished(std::forward<ContinueFunc>(f)(std::forward<Args>(a)...).status());
Review comment:
That could maybe collapse two of these cases. Although that middle overload is a partial function specialization which I don't think is allowed. I'm not sure it ends up being any simpler. I think this was more necessary with the two classes approach.
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -691,34 +876,41 @@ struct Continue {
}
};
-template <typename T = detail::Empty>
+template <typename T = internal::Empty>
util::optional<T> Break(T break_value = {}) {
return util::optional<T>{std::move(break_value)};
}
-template <typename T = detail::Empty>
+template <typename T = internal::Empty>
using ControlFlow = util::optional<T>;
+template <typename T>
+void ForwardControlResult(const Result<ControlFlow<T>>& result, Future<T> sink) {
+ sink.MarkFinished(**result);
+}
+template <>
+inline void ForwardControlResult(const Result<ControlFlow<>>& result, Future<> sink) {
+ sink.MarkFinished();
Review comment:
This has gone away.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r625263718
##########
File path: cpp/src/arrow/util/thread_pool_test.cc
##########
@@ -189,11 +189,7 @@ TEST_P(TestRunSynchronously, SpawnMoreNested) {
auto top_level_task = [&](Executor* executor) -> Future<> {
auto fut_a = DeferNotOk(executor->Submit([&] { nested_ran++; }));
auto fut_b = DeferNotOk(executor->Submit([&] { nested_ran++; }));
- return AllComplete({fut_a, fut_b})
- .Then([&](const Result<arrow::detail::Empty>& result) {
- nested_ran++;
- return result;
- });
+ return AllComplete({fut_a, fut_b}).Then([&]() { nested_ran++; });
Review comment:
What a continuation returns is a separate decision from what it accepts. However, returning a result here was essentially a no-op before and would still be one now.
The first argument (or only argument in the 1-argument overload) to `Then` will only be called on success and it is given `result.ValueOrDie()` so accepting a `Result` is somewhat counter-productive. This is different from `AddCallback` which is called on success or failure.
It would be good to make it a compile error to accept a `Result<T>` in all cases of `Then` to avoid this confusion. The current state of the PR will make a compile error for `Future<Empty>::Then` (which is why this was caught) so I'll update it to catch the `Future<T>::Then` case as well.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r626227972
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -56,6 +47,39 @@ struct is_future<Future<T>> : std::true_type {};
template <typename Signature>
using result_of_t = typename std::result_of<Signature>::type;
+template <typename Source, typename Dest>
+typename std::enable_if<Source::is_empty>::type Propagate(Source& source, Dest dest) {
Review comment:
Changed to a pointer.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#issuecomment-840311677
Ok, using @bkietz 's suggestions I was able to collapse the two classes back into one. I still have some cleanup to do so this is not ready for review yet. The result is more compact, although I'm not necessarily convinced it is any more maintainable but :shrug: .
--
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r626751707
##########
File path: cpp/src/arrow/util/thread_pool.h
##########
@@ -227,14 +228,14 @@ class ARROW_EXPORT SerialExecutor : public Executor {
std::shared_ptr<State> state_;
template <typename T>
- Result<T> Run(TopLevelTask<T> initial_task) {
+ Future<T> Run(TopLevelTask<T> initial_task) {
auto final_fut = std::move(initial_task)(this);
if (final_fut.is_finished()) {
- return final_fut.result();
+ return final_fut;
}
- final_fut.AddCallback([this](const Result<T>&) { MarkFinished(); });
+ final_fut.AddCallback([this](...) { MarkFinished(); });
Review comment:
`...` is not a template, and in particular is distinct from a parameter pack. Ellipsis indicates a [variadic function](https://en.cppreference.com/w/cpp/language/variadic_arguments) (whose arguments must be accessed with `va_start` and friends)
--
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:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r629340021
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -78,28 +110,32 @@ struct ContinueFuture {
template <typename ContinueFunc, typename... Args,
typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
typename NextFuture = ForReturn<ContinueResult>>
- typename std::enable_if<!std::is_void<ContinueResult>::value &&
- !is_future<ContinueResult>::value>::type
+ typename std::enable_if<
+ !std::is_void<ContinueResult>::value && !is_future<ContinueResult>::value &&
+ (!NextFuture::is_empty || std::is_same<ContinueResult, Status>::value)>::type
Review comment:
You should add comments before each of these overloads because it's getting very hard to guess what their role is.
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -56,6 +47,47 @@ struct is_future<Future<T>> : std::true_type {};
template <typename Signature>
using result_of_t = typename std::result_of<Signature>::type;
+template <typename Source, typename Dest>
+typename std::enable_if<Source::is_empty>::type Propagate(Source* source, Dest dest) {
Review comment:
Is this used somewhere?
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -322,56 +341,98 @@ class ARROW_MUST_USE_TYPE Future {
return impl_->Wait(seconds);
}
- // Producer API
+ protected:
+ void InitializeFromResult(Result<ValueType> res) {
+ if (ARROW_PREDICT_TRUE(res.ok())) {
+ impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS);
+ } else {
+ impl_ = FutureImpl::MakeFinished(FutureState::FAILURE);
+ }
+ SetResult(std::move(res));
+ }
- /// \brief Producer API: mark Future finished
- ///
- /// The Future's result is set to `res`.
- void MarkFinished(Result<ValueType> res) { DoMarkFinished(std::move(res)); }
+ void Initialize() { impl_ = FutureImpl::Make(); }
- /// \brief Mark a Future<> completed with the provided Status.
- template <typename E = ValueType, typename = typename std::enable_if<
- std::is_same<E, detail::Empty>::value>::type>
- void MarkFinished(Status s = Status::OK()) {
- return DoMarkFinished(E::ToResult(std::move(s)));
+ Result<ValueType>* GetResult() const {
+ return static_cast<Result<ValueType>*>(impl_->result_.get());
}
- /// \brief Producer API: instantiate a valid Future
- ///
- /// The Future's state is initialized with PENDING. If you are creating a future with
- /// this method you must ensure that future is eventually completed (with success or
- /// failure). Creating a future, returning it, and never completing the future can lead
- /// to memory leaks (for example, see Loop).
- static Future Make() {
- Future fut;
- fut.impl_ = FutureImpl::Make();
- return fut;
+ void SetResult(Result<ValueType> res) {
+ impl_->result_ = {new Result<ValueType>(std::move(res)),
+ [](void* p) { delete static_cast<Result<ValueType>*>(p); }};
}
- /// \brief Producer API: instantiate a finished Future
- static Future MakeFinished(Result<ValueType> res) {
- Future fut;
- if (ARROW_PREDICT_TRUE(res.ok())) {
- fut.impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS);
+ void DoMarkFinished(Result<ValueType> res) {
+ SetResult(std::move(res));
+
+ if (ARROW_PREDICT_TRUE(GetResult()->ok())) {
+ impl_->MarkFinished();
} else {
- fut.impl_ = FutureImpl::MakeFinished(FutureState::FAILURE);
+ impl_->MarkFailed();
}
- fut.SetResult(std::move(res));
- return fut;
}
- /// \brief Make a finished Future<> with the provided Status.
- template <typename E = ValueType, typename = typename std::enable_if<
- std::is_same<E, detail::Empty>::value>::type>
- static Future<> MakeFinished(Status s = Status::OK()) {
- return MakeFinished(E::ToResult(std::move(s)));
+ void CheckValid() const {
+#ifndef NDEBUG
+ if (!is_valid()) {
+ Status::Invalid("Invalid Future (default-initialized?)").Abort();
+ }
+#endif
+ }
+
+ explicit FutureBase(std::shared_ptr<FutureImpl> impl) : impl_(std::move(impl)) {}
+
+ std::shared_ptr<FutureImpl> impl_;
+
+ friend class FutureWaiter;
+ friend struct detail::ContinueFuture;
+};
+
+template <typename T>
+class ARROW_MUST_USE_TYPE Future : public FutureBase<T> {
+ public:
+ using ValueType = T;
+ using SyncType = Result<T>;
+ static constexpr bool is_empty = false;
+
+ Future() = default;
+
+ /// \brief Returns an rvalue to the result. This method is potentially unsafe
+ ///
+ /// The future is not the unique owner of the result, copies of a future will
+ /// also point to the same result. You must make sure that no other copies
+ /// of the future exist. Attempts to add callbacks after you move the result
+ /// will result in undefined behavior.
+ Result<ValueType>&& MoveResult() {
+ FutureBase<T>::Wait();
+ return std::move(*FutureBase<T>::GetResult());
+ }
+
+ /// \brief Wait for the Future to complete and return its Result (or Status for an empty
+ /// future)
+ ///
+ /// This method is useful for general purpose code converting from async to sync where T
+ /// is a template parameter and may be empty.
+ const SyncType& to_sync() const { return FutureBase<T>::result(); }
Review comment:
So this is just the `result()` method? Is there an actual need in introducing this?
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -322,56 +341,98 @@ class ARROW_MUST_USE_TYPE Future {
return impl_->Wait(seconds);
}
- // Producer API
+ protected:
+ void InitializeFromResult(Result<ValueType> res) {
+ if (ARROW_PREDICT_TRUE(res.ok())) {
+ impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS);
+ } else {
+ impl_ = FutureImpl::MakeFinished(FutureState::FAILURE);
+ }
+ SetResult(std::move(res));
+ }
- /// \brief Producer API: mark Future finished
- ///
- /// The Future's result is set to `res`.
- void MarkFinished(Result<ValueType> res) { DoMarkFinished(std::move(res)); }
+ void Initialize() { impl_ = FutureImpl::Make(); }
- /// \brief Mark a Future<> completed with the provided Status.
- template <typename E = ValueType, typename = typename std::enable_if<
- std::is_same<E, detail::Empty>::value>::type>
- void MarkFinished(Status s = Status::OK()) {
- return DoMarkFinished(E::ToResult(std::move(s)));
+ Result<ValueType>* GetResult() const {
+ return static_cast<Result<ValueType>*>(impl_->result_.get());
}
- /// \brief Producer API: instantiate a valid Future
- ///
- /// The Future's state is initialized with PENDING. If you are creating a future with
- /// this method you must ensure that future is eventually completed (with success or
- /// failure). Creating a future, returning it, and never completing the future can lead
- /// to memory leaks (for example, see Loop).
- static Future Make() {
- Future fut;
- fut.impl_ = FutureImpl::Make();
- return fut;
+ void SetResult(Result<ValueType> res) {
+ impl_->result_ = {new Result<ValueType>(std::move(res)),
+ [](void* p) { delete static_cast<Result<ValueType>*>(p); }};
}
- /// \brief Producer API: instantiate a finished Future
- static Future MakeFinished(Result<ValueType> res) {
- Future fut;
- if (ARROW_PREDICT_TRUE(res.ok())) {
- fut.impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS);
+ void DoMarkFinished(Result<ValueType> res) {
+ SetResult(std::move(res));
+
+ if (ARROW_PREDICT_TRUE(GetResult()->ok())) {
+ impl_->MarkFinished();
} else {
- fut.impl_ = FutureImpl::MakeFinished(FutureState::FAILURE);
+ impl_->MarkFailed();
}
- fut.SetResult(std::move(res));
- return fut;
}
- /// \brief Make a finished Future<> with the provided Status.
- template <typename E = ValueType, typename = typename std::enable_if<
- std::is_same<E, detail::Empty>::value>::type>
- static Future<> MakeFinished(Status s = Status::OK()) {
- return MakeFinished(E::ToResult(std::move(s)));
+ void CheckValid() const {
+#ifndef NDEBUG
+ if (!is_valid()) {
+ Status::Invalid("Invalid Future (default-initialized?)").Abort();
+ }
+#endif
+ }
+
+ explicit FutureBase(std::shared_ptr<FutureImpl> impl) : impl_(std::move(impl)) {}
+
+ std::shared_ptr<FutureImpl> impl_;
+
+ friend class FutureWaiter;
+ friend struct detail::ContinueFuture;
+};
+
+template <typename T>
+class ARROW_MUST_USE_TYPE Future : public FutureBase<T> {
+ public:
+ using ValueType = T;
+ using SyncType = Result<T>;
Review comment:
Call this `ResultType`?
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -56,6 +47,47 @@ struct is_future<Future<T>> : std::true_type {};
template <typename Signature>
using result_of_t = typename std::result_of<Signature>::type;
+template <typename Source, typename Dest>
+typename std::enable_if<Source::is_empty>::type Propagate(Source* source, Dest dest) {
+ struct MarkNextFinished {
+ void operator()(const Status& status) && { next.MarkFinished(status); }
+ Dest next;
+ };
+ source->AddCallback(MarkNextFinished{std::move(dest)});
+}
+
+template <typename Source, typename Dest, bool SourceEmpty = Source::is_empty,
+ bool DestEmpty = Dest::is_empty>
+struct MarkNextFinished {};
+
+template <typename Source, typename Dest>
+struct MarkNextFinished<Source, Dest, true, false> {
+ void operator()(const Status& status) && { next.MarkFinished(status); }
Review comment:
Is this one valid? If the status is ok, then `next` needs an actual `Result` object.
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -78,28 +110,32 @@ struct ContinueFuture {
template <typename ContinueFunc, typename... Args,
typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
typename NextFuture = ForReturn<ContinueResult>>
- typename std::enable_if<!std::is_void<ContinueResult>::value &&
- !is_future<ContinueResult>::value>::type
+ typename std::enable_if<
+ !std::is_void<ContinueResult>::value && !is_future<ContinueResult>::value &&
+ (!NextFuture::is_empty || std::is_same<ContinueResult, Status>::value)>::type
operator()(NextFuture next, ContinueFunc&& f, Args&&... a) const {
next.MarkFinished(std::forward<ContinueFunc>(f)(std::forward<Args>(a)...));
}
+ template <typename ContinueFunc, typename... Args,
+ typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
+ typename NextFuture = ForReturn<ContinueResult>>
+ typename std::enable_if<!std::is_void<ContinueResult>::value &&
+ !is_future<ContinueResult>::value && NextFuture::is_empty &&
+ !std::is_same<ContinueResult, Status>::value>::type
+ operator()(NextFuture next, ContinueFunc&& f, Args&&... a) const {
+ next.MarkFinished(std::forward<ContinueFunc>(f)(std::forward<Args>(a)...).status());
Review comment:
Hmm... so why not create a specialized helper such as:
```c++
void ForwardResult(const Result<T>&, Future<U>* fut);
void ForwardResult(const Result<T>&, Future<>* fut);
void ForwardResult(const Status&, Future<>* fut);
```
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -691,34 +876,41 @@ struct Continue {
}
};
-template <typename T = detail::Empty>
+template <typename T = internal::Empty>
util::optional<T> Break(T break_value = {}) {
return util::optional<T>{std::move(break_value)};
}
-template <typename T = detail::Empty>
+template <typename T = internal::Empty>
using ControlFlow = util::optional<T>;
+template <typename T>
+void ForwardControlResult(const Result<ControlFlow<T>>& result, Future<T> sink) {
+ sink.MarkFinished(**result);
+}
+template <>
+inline void ForwardControlResult(const Result<ControlFlow<>>& result, Future<> sink) {
+ sink.MarkFinished();
Review comment:
I'm a bit skeptical that we have to introduce this yet another result-to-future forwarding primitive.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r625504477
##########
File path: cpp/src/arrow/util/thread_pool_test.cc
##########
@@ -189,11 +189,7 @@ TEST_P(TestRunSynchronously, SpawnMoreNested) {
auto top_level_task = [&](Executor* executor) -> Future<> {
auto fut_a = DeferNotOk(executor->Submit([&] { nested_ran++; }));
auto fut_b = DeferNotOk(executor->Submit([&] { nested_ran++; }));
- return AllComplete({fut_a, fut_b})
- .Then([&](const Result<arrow::detail::Empty>& result) {
- nested_ran++;
- return result;
- });
+ return AllComplete({fut_a, fut_b}).Then([&]() { nested_ran++; });
Review comment:
I took a stab at preventing `Then` callbacks accepting `Result<T>` but didn't make much headway and so I'd prefer to defer that to a future PR. The problem is that we have `Args` in `ContinueFuture` but no `T` to check if `Args[0] == Result<T>`. In the `Then` method we have `T` but no idea what `Args` are.
I suspect there is someway to test if `Args[0]` is a `Result<?>` but I tried a few things and didn't figure it out.
--
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#issuecomment-839985461
Instead of sniffing the class' template parameter, it'd be straightforward to check a callback's argument type.
in `AddCallback(on_complete)`:
- if `on_complete` takes a Status, let it be completed with just the status (ignoring the value)
- otherwise, let it be completed with the Result
in `Then(on_success, on_failure)`:
- if `on_success` takes no args, let it be wrapped in an on_success which ignores the value
- otherwise, let it receive the value
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r626237982
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -691,20 +862,29 @@ struct Continue {
}
};
-template <typename T = detail::Empty>
+template <typename T = internal::Empty>
util::optional<T> Break(T break_value = {}) {
return util::optional<T>{std::move(break_value)};
}
-template <typename T = detail::Empty>
+template <typename T = internal::Empty>
using ControlFlow = util::optional<T>;
+template <typename T>
+void ForwardControlResult(const Result<ControlFlow<T>>& result, Future<T>& sink) {
Review comment:
At this point we're done with `break_fut` so I changed it to move `break_fut` and got rid of the `&`.
--
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:
users@infra.apache.org
[GitHub] [arrow] bkietz closed pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #10205:
URL: https://github.com/apache/arrow/pull/10205
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#issuecomment-832573802
I rebased and I believe I have addressed all the PR feedback up to date.
--
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:
users@infra.apache.org
[GitHub] [arrow] bkietz closed pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #10205:
URL: https://github.com/apache/arrow/pull/10205
--
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r625876665
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -78,28 +102,31 @@ struct ContinueFuture {
template <typename ContinueFunc, typename... Args,
typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
typename NextFuture = ForReturn<ContinueResult>>
- typename std::enable_if<!std::is_void<ContinueResult>::value &&
- !is_future<ContinueResult>::value>::type
+ typename std::enable_if<
+ !std::is_void<ContinueResult>::value && !is_future<ContinueResult>::value &&
+ (!NextFuture::is_empty || std::is_same<ContinueResult, Status>::value)>::type
operator()(NextFuture next, ContinueFunc&& f, Args&&... a) const {
next.MarkFinished(std::forward<ContinueFunc>(f)(std::forward<Args>(a)...));
}
+ template <typename ContinueFunc, typename... Args,
+ typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
+ typename NextFuture = ForReturn<ContinueResult>>
+ typename std::enable_if<!std::is_void<ContinueResult>::value &&
+ !is_future<ContinueResult>::value && NextFuture::is_empty &&
+ !std::is_same<ContinueResult, Status>::value>::type
+ operator()(NextFuture next, ContinueFunc&& f, Args&&... a) const {
+ next.MarkFinished(std::forward<ContinueFunc>(f)(std::forward<Args>(a)...).status());
+ }
+
template <typename ContinueFunc, typename... Args,
typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
typename NextFuture = ForReturn<ContinueResult>>
typename std::enable_if<is_future<ContinueResult>::value>::type operator()(
NextFuture next, ContinueFunc&& f, Args&&... a) const {
ContinueResult signal_to_complete_next =
std::forward<ContinueFunc>(f)(std::forward<Args>(a)...);
-
- struct MarkNextFinished {
- void operator()(const Result<typename ContinueResult::ValueType>& result) && {
- next.MarkFinished(result);
- }
- NextFuture next;
- };
-
- signal_to_complete_next.AddCallback(MarkNextFinished{std::move(next)});
+ Propagate<ContinueResult, NextFuture>(signal_to_complete_next, std::move(next));
Review comment:
It might be more clear to only extract a callback factory, rather than calling AddCallback in Propagate:
```suggestion
auto callback = MakeMarkNextFinished<ContinueResult>(std::move(next));
signal_to_complete_next.AddCallback(std::move(callback));
```
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -56,6 +47,39 @@ struct is_future<Future<T>> : std::true_type {};
template <typename Signature>
using result_of_t = typename std::result_of<Signature>::type;
+template <typename Source, typename Dest>
+typename std::enable_if<Source::is_empty>::type Propagate(Source& source, Dest dest) {
Review comment:
Style nit: mutable references should be avoided
```suggestion
typename std::enable_if<Source::is_empty>::type Propagate(Source* source, Dest dest) {
```
##########
File path: cpp/src/arrow/util/thread_pool.h
##########
@@ -227,14 +228,14 @@ class ARROW_EXPORT SerialExecutor : public Executor {
std::shared_ptr<State> state_;
template <typename T>
- Result<T> Run(TopLevelTask<T> initial_task) {
+ Future<T> Run(TopLevelTask<T> initial_task) {
auto final_fut = std::move(initial_task)(this);
if (final_fut.is_finished()) {
- return final_fut.result();
+ return final_fut;
}
- final_fut.AddCallback([this](const Result<T>&) { MarkFinished(); });
+ final_fut.AddCallback([this](...) { MarkFinished(); });
Review comment:
`...` can only be used to ignore trivially copyable/destructible arguments, so I'd revert this
```suggestion
final_fut.AddCallback([this](const Result<T>&) { MarkFinished(); });
```
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -691,20 +862,29 @@ struct Continue {
}
};
-template <typename T = detail::Empty>
+template <typename T = internal::Empty>
util::optional<T> Break(T break_value = {}) {
return util::optional<T>{std::move(break_value)};
}
-template <typename T = detail::Empty>
+template <typename T = internal::Empty>
using ControlFlow = util::optional<T>;
+template <typename T>
+void ForwardControlResult(const Result<ControlFlow<T>>& result, Future<T>& sink) {
Review comment:
```suggestion
void ForwardControlResult(const Result<ControlFlow<T>>& result, Future<T>* sink) {
```
##########
File path: cpp/src/arrow/util/thread_pool_test.cc
##########
@@ -189,11 +189,7 @@ TEST_P(TestRunSynchronously, SpawnMoreNested) {
auto top_level_task = [&](Executor* executor) -> Future<> {
auto fut_a = DeferNotOk(executor->Submit([&] { nested_ran++; }));
auto fut_b = DeferNotOk(executor->Submit([&] { nested_ran++; }));
- return AllComplete({fut_a, fut_b})
- .Then([&](const Result<arrow::detail::Empty>& result) {
- nested_ran++;
- return result;
- });
+ return AllComplete({fut_a, fut_b}).Then([&]() { nested_ran++; });
Review comment:
you could add the following assertion to the body of Then:
```c++
using OnSuccessArg =
typename std::decay<internal::call_traits::argument_type<0, OnSuccess>>::type;
static_assert(
!std::is_same<OnSuccessArg, typename EnsureResult<OnSuccessArg>::type>::value,
"OnSuccess' argument should not be a Result");
```
Currently `call_traits::argument_type` breaks when trying to examine an ellipsis, so the following wouldn't compile anymore:
```c++
int_future.Then([](...) {});
```
But that'd probably be an acceptable loss
--
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:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r625111464
##########
File path: cpp/src/arrow/util/thread_pool_test.cc
##########
@@ -189,11 +189,7 @@ TEST_P(TestRunSynchronously, SpawnMoreNested) {
auto top_level_task = [&](Executor* executor) -> Future<> {
auto fut_a = DeferNotOk(executor->Submit([&] { nested_ran++; }));
auto fut_b = DeferNotOk(executor->Submit([&] { nested_ran++; }));
- return AllComplete({fut_a, fut_b})
- .Then([&](const Result<arrow::detail::Empty>& result) {
- nested_ran++;
- return result;
- });
+ return AllComplete({fut_a, fut_b}).Then([&]() { nested_ran++; });
Review comment:
I'm curious why the old version took a `Result` and returned it. Was there a discrepancy between `Future<T>::Then` and `Future<Empty>::Then`?
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r626235120
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -78,28 +102,31 @@ struct ContinueFuture {
template <typename ContinueFunc, typename... Args,
typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
typename NextFuture = ForReturn<ContinueResult>>
- typename std::enable_if<!std::is_void<ContinueResult>::value &&
- !is_future<ContinueResult>::value>::type
+ typename std::enable_if<
+ !std::is_void<ContinueResult>::value && !is_future<ContinueResult>::value &&
+ (!NextFuture::is_empty || std::is_same<ContinueResult, Status>::value)>::type
operator()(NextFuture next, ContinueFunc&& f, Args&&... a) const {
next.MarkFinished(std::forward<ContinueFunc>(f)(std::forward<Args>(a)...));
}
+ template <typename ContinueFunc, typename... Args,
+ typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
+ typename NextFuture = ForReturn<ContinueResult>>
+ typename std::enable_if<!std::is_void<ContinueResult>::value &&
+ !is_future<ContinueResult>::value && NextFuture::is_empty &&
+ !std::is_same<ContinueResult, Status>::value>::type
+ operator()(NextFuture next, ContinueFunc&& f, Args&&... a) const {
+ next.MarkFinished(std::forward<ContinueFunc>(f)(std::forward<Args>(a)...).status());
+ }
+
template <typename ContinueFunc, typename... Args,
typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
typename NextFuture = ForReturn<ContinueResult>>
typename std::enable_if<is_future<ContinueResult>::value>::type operator()(
NextFuture next, ContinueFunc&& f, Args&&... a) const {
ContinueResult signal_to_complete_next =
std::forward<ContinueFunc>(f)(std::forward<Args>(a)...);
-
- struct MarkNextFinished {
- void operator()(const Result<typename ContinueResult::ValueType>& result) && {
- next.MarkFinished(result);
- }
- NextFuture next;
- };
-
- signal_to_complete_next.AddCallback(MarkNextFinished{std::move(next)});
+ Propagate<ContinueResult, NextFuture>(signal_to_complete_next, std::move(next));
Review comment:
I tried this for a bit but couldn't figure out what the return type of `MakeMarkNextFinished` should be. Then I realized I could pull the struct out of the `Propagate` method with template specialization.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r631576939
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -322,56 +341,98 @@ class ARROW_MUST_USE_TYPE Future {
return impl_->Wait(seconds);
}
- // Producer API
+ protected:
+ void InitializeFromResult(Result<ValueType> res) {
+ if (ARROW_PREDICT_TRUE(res.ok())) {
+ impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS);
+ } else {
+ impl_ = FutureImpl::MakeFinished(FutureState::FAILURE);
+ }
+ SetResult(std::move(res));
+ }
- /// \brief Producer API: mark Future finished
- ///
- /// The Future's result is set to `res`.
- void MarkFinished(Result<ValueType> res) { DoMarkFinished(std::move(res)); }
+ void Initialize() { impl_ = FutureImpl::Make(); }
- /// \brief Mark a Future<> completed with the provided Status.
- template <typename E = ValueType, typename = typename std::enable_if<
- std::is_same<E, detail::Empty>::value>::type>
- void MarkFinished(Status s = Status::OK()) {
- return DoMarkFinished(E::ToResult(std::move(s)));
+ Result<ValueType>* GetResult() const {
+ return static_cast<Result<ValueType>*>(impl_->result_.get());
}
- /// \brief Producer API: instantiate a valid Future
- ///
- /// The Future's state is initialized with PENDING. If you are creating a future with
- /// this method you must ensure that future is eventually completed (with success or
- /// failure). Creating a future, returning it, and never completing the future can lead
- /// to memory leaks (for example, see Loop).
- static Future Make() {
- Future fut;
- fut.impl_ = FutureImpl::Make();
- return fut;
+ void SetResult(Result<ValueType> res) {
+ impl_->result_ = {new Result<ValueType>(std::move(res)),
+ [](void* p) { delete static_cast<Result<ValueType>*>(p); }};
}
- /// \brief Producer API: instantiate a finished Future
- static Future MakeFinished(Result<ValueType> res) {
- Future fut;
- if (ARROW_PREDICT_TRUE(res.ok())) {
- fut.impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS);
+ void DoMarkFinished(Result<ValueType> res) {
+ SetResult(std::move(res));
+
+ if (ARROW_PREDICT_TRUE(GetResult()->ok())) {
+ impl_->MarkFinished();
} else {
- fut.impl_ = FutureImpl::MakeFinished(FutureState::FAILURE);
+ impl_->MarkFailed();
}
- fut.SetResult(std::move(res));
- return fut;
}
- /// \brief Make a finished Future<> with the provided Status.
- template <typename E = ValueType, typename = typename std::enable_if<
- std::is_same<E, detail::Empty>::value>::type>
- static Future<> MakeFinished(Status s = Status::OK()) {
- return MakeFinished(E::ToResult(std::move(s)));
+ void CheckValid() const {
+#ifndef NDEBUG
+ if (!is_valid()) {
+ Status::Invalid("Invalid Future (default-initialized?)").Abort();
+ }
+#endif
+ }
+
+ explicit FutureBase(std::shared_ptr<FutureImpl> impl) : impl_(std::move(impl)) {}
+
+ std::shared_ptr<FutureImpl> impl_;
+
+ friend class FutureWaiter;
+ friend struct detail::ContinueFuture;
+};
+
+template <typename T>
+class ARROW_MUST_USE_TYPE Future : public FutureBase<T> {
+ public:
+ using ValueType = T;
+ using SyncType = Result<T>;
Review comment:
`SyncType` is the synchronous analogue of the future. For `Future<T>` it is `Result<T>` and for `Future<>` it is `Status`.
Any code that needs to convert from asynchronous to synchronous and does not know the type of `T` will need to rely on this. It's currently used in `arrow::internal::RunSynchronously`
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r631577351
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -322,56 +341,98 @@ class ARROW_MUST_USE_TYPE Future {
return impl_->Wait(seconds);
}
- // Producer API
+ protected:
+ void InitializeFromResult(Result<ValueType> res) {
+ if (ARROW_PREDICT_TRUE(res.ok())) {
+ impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS);
+ } else {
+ impl_ = FutureImpl::MakeFinished(FutureState::FAILURE);
+ }
+ SetResult(std::move(res));
+ }
- /// \brief Producer API: mark Future finished
- ///
- /// The Future's result is set to `res`.
- void MarkFinished(Result<ValueType> res) { DoMarkFinished(std::move(res)); }
+ void Initialize() { impl_ = FutureImpl::Make(); }
- /// \brief Mark a Future<> completed with the provided Status.
- template <typename E = ValueType, typename = typename std::enable_if<
- std::is_same<E, detail::Empty>::value>::type>
- void MarkFinished(Status s = Status::OK()) {
- return DoMarkFinished(E::ToResult(std::move(s)));
+ Result<ValueType>* GetResult() const {
+ return static_cast<Result<ValueType>*>(impl_->result_.get());
}
- /// \brief Producer API: instantiate a valid Future
- ///
- /// The Future's state is initialized with PENDING. If you are creating a future with
- /// this method you must ensure that future is eventually completed (with success or
- /// failure). Creating a future, returning it, and never completing the future can lead
- /// to memory leaks (for example, see Loop).
- static Future Make() {
- Future fut;
- fut.impl_ = FutureImpl::Make();
- return fut;
+ void SetResult(Result<ValueType> res) {
+ impl_->result_ = {new Result<ValueType>(std::move(res)),
+ [](void* p) { delete static_cast<Result<ValueType>*>(p); }};
}
- /// \brief Producer API: instantiate a finished Future
- static Future MakeFinished(Result<ValueType> res) {
- Future fut;
- if (ARROW_PREDICT_TRUE(res.ok())) {
- fut.impl_ = FutureImpl::MakeFinished(FutureState::SUCCESS);
+ void DoMarkFinished(Result<ValueType> res) {
+ SetResult(std::move(res));
+
+ if (ARROW_PREDICT_TRUE(GetResult()->ok())) {
+ impl_->MarkFinished();
} else {
- fut.impl_ = FutureImpl::MakeFinished(FutureState::FAILURE);
+ impl_->MarkFailed();
}
- fut.SetResult(std::move(res));
- return fut;
}
- /// \brief Make a finished Future<> with the provided Status.
- template <typename E = ValueType, typename = typename std::enable_if<
- std::is_same<E, detail::Empty>::value>::type>
- static Future<> MakeFinished(Status s = Status::OK()) {
- return MakeFinished(E::ToResult(std::move(s)));
+ void CheckValid() const {
+#ifndef NDEBUG
+ if (!is_valid()) {
+ Status::Invalid("Invalid Future (default-initialized?)").Abort();
+ }
+#endif
+ }
+
+ explicit FutureBase(std::shared_ptr<FutureImpl> impl) : impl_(std::move(impl)) {}
+
+ std::shared_ptr<FutureImpl> impl_;
+
+ friend class FutureWaiter;
+ friend struct detail::ContinueFuture;
+};
+
+template <typename T>
+class ARROW_MUST_USE_TYPE Future : public FutureBase<T> {
+ public:
+ using ValueType = T;
+ using SyncType = Result<T>;
+ static constexpr bool is_empty = false;
+
+ Future() = default;
+
+ /// \brief Returns an rvalue to the result. This method is potentially unsafe
+ ///
+ /// The future is not the unique owner of the result, copies of a future will
+ /// also point to the same result. You must make sure that no other copies
+ /// of the future exist. Attempts to add callbacks after you move the result
+ /// will result in undefined behavior.
+ Result<ValueType>&& MoveResult() {
+ FutureBase<T>::Wait();
+ return std::move(*FutureBase<T>::GetResult());
+ }
+
+ /// \brief Wait for the Future to complete and return its Result (or Status for an empty
+ /// future)
+ ///
+ /// This method is useful for general purpose code converting from async to sync where T
+ /// is a template parameter and may be empty.
+ const SyncType& to_sync() const { return FutureBase<T>::result(); }
Review comment:
So based on the other comment this will return either `result()` or `status()` depending on `T`. It's the opposite of the "result-to-future" forwarding primitive. Note: after switching to one class this became a standalone static method `FutureToSync`.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r631921805
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -78,28 +110,32 @@ struct ContinueFuture {
template <typename ContinueFunc, typename... Args,
typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
typename NextFuture = ForReturn<ContinueResult>>
- typename std::enable_if<!std::is_void<ContinueResult>::value &&
- !is_future<ContinueResult>::value>::type
+ typename std::enable_if<
+ !std::is_void<ContinueResult>::value && !is_future<ContinueResult>::value &&
+ (!NextFuture::is_empty || std::is_same<ContinueResult, Status>::value)>::type
Review comment:
I added some comments helping to explain how each are different.
--
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#issuecomment-829682096
https://issues.apache.org/jira/browse/ARROW-12004
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r631921595
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -56,6 +47,47 @@ struct is_future<Future<T>> : std::true_type {};
template <typename Signature>
using result_of_t = typename std::result_of<Signature>::type;
+template <typename Source, typename Dest>
+typename std::enable_if<Source::is_empty>::type Propagate(Source* source, Dest dest) {
Review comment:
No. I have removed it.
--
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:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#issuecomment-839549165
Hmm, ok, I may be hoping too much from C++ :-) I'll let @bkietz chime in.
--
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:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10205: ARROW-12004: [C++] Result is annoying
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10205:
URL: https://github.com/apache/arrow/pull/10205#discussion_r626235120
##########
File path: cpp/src/arrow/util/future.h
##########
@@ -78,28 +102,31 @@ struct ContinueFuture {
template <typename ContinueFunc, typename... Args,
typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
typename NextFuture = ForReturn<ContinueResult>>
- typename std::enable_if<!std::is_void<ContinueResult>::value &&
- !is_future<ContinueResult>::value>::type
+ typename std::enable_if<
+ !std::is_void<ContinueResult>::value && !is_future<ContinueResult>::value &&
+ (!NextFuture::is_empty || std::is_same<ContinueResult, Status>::value)>::type
operator()(NextFuture next, ContinueFunc&& f, Args&&... a) const {
next.MarkFinished(std::forward<ContinueFunc>(f)(std::forward<Args>(a)...));
}
+ template <typename ContinueFunc, typename... Args,
+ typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
+ typename NextFuture = ForReturn<ContinueResult>>
+ typename std::enable_if<!std::is_void<ContinueResult>::value &&
+ !is_future<ContinueResult>::value && NextFuture::is_empty &&
+ !std::is_same<ContinueResult, Status>::value>::type
+ operator()(NextFuture next, ContinueFunc&& f, Args&&... a) const {
+ next.MarkFinished(std::forward<ContinueFunc>(f)(std::forward<Args>(a)...).status());
+ }
+
template <typename ContinueFunc, typename... Args,
typename ContinueResult = result_of_t<ContinueFunc && (Args && ...)>,
typename NextFuture = ForReturn<ContinueResult>>
typename std::enable_if<is_future<ContinueResult>::value>::type operator()(
NextFuture next, ContinueFunc&& f, Args&&... a) const {
ContinueResult signal_to_complete_next =
std::forward<ContinueFunc>(f)(std::forward<Args>(a)...);
-
- struct MarkNextFinished {
- void operator()(const Result<typename ContinueResult::ValueType>& result) && {
- next.MarkFinished(result);
- }
- NextFuture next;
- };
-
- signal_to_complete_next.AddCallback(MarkNextFinished{std::move(next)});
+ Propagate<ContinueResult, NextFuture>(signal_to_complete_next, std::move(next));
Review comment:
I tried this for a bit but couldn't figure out what the return type of `MakeMarkNextFinished` should be. Then I realized I could pull the struct out of the `Propagate` method with template specialization. I don't know if it's "more clear" (it's still C++ after all) but it avoids adding another function call which is good given the stack traces are already pretty rough for futures.
--
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:
users@infra.apache.org