Add getTry back into folly::Future, with blocking behaviour consistent with...

Add getTry back into folly::Future, with blocking behaviour consistent with get() and with SemiFuture::getTry().

Summary:
Formerly Future::getTry was non-blocking, and exhibited confusing behaviour of throwing on timeouts, but returning a try containing exceptions related to the workload of the future. This was inconsistent with Future::get, and thus surprising.

An earlier change safely removed this broken version of the operation. Here we add it back in with consistent behaviour.

Reviewed By: yfeldblum

Differential Revision: D23477042

fbshipit-source-id: afda6b37c86531dab43ac31ec6d605cc6c5d670c

folly/futures/Future-inl.h

@@ -2360,18 +2360,22 @@ Future<T>&& Future<T>::waitVia(
 
 template <class T>
 T Future<T>::get() && {
-  wait();
-  return copy(std::move(*this)).value();
+  return std::move(*this).getTry().value();
 }
 
 template <class T>
 T Future<T>::get(HighResDuration dur) && {
-  wait(dur);
-  auto future = copy(std::move(*this));
-  if (!future.isReady()) {
-    throw_exception<FutureTimeout>();
-  }
-  return std::move(future).value();
+  return std::move(*this).getTry(dur).value();
+}
+
+template <class T>
+Try<T> Future<T>::getTry() && {
+  return std::move(*this).semi().getTry();
+}
+
+template <class T>
+Try<T> Future<T>::getTry(HighResDuration dur) && {
+  return std::move(*this).semi().getTry(dur);
 }
 
 template <class T>

folly/futures/Future.h

@@ -1732,6 +1732,31 @@ class Future : private futures::detail::FutureBase<T> {
   /// - `valid() == false`
   T get(HighResDuration dur) &&;
 
+  /// Blocks until the future is fulfilled. Returns the Try of the result
+  ///   (moved-out).
+  ///
+  /// Preconditions:
+  ///
+  /// - `valid() == true` (else throws FutureInvalid)
+  ///
+  /// Postconditions:
+  ///
+  /// - `valid() == false`
+  Try<T> getTry() &&;
+
+  /// Blocks until the future is fulfilled, or until `dur` elapses.
+  /// Returns the Try of the result (moved-out), or throws FutureTimeout
+  /// exception.
+  ///
+  /// Preconditions:
+  ///
+  /// - `valid() == true` (else throws FutureInvalid)
+  ///
+  /// Postconditions:
+  ///
+  /// - `valid() == false`
+  Try<T> getTry(HighResDuration dur) &&;
+
   /// Blocks until this Future is complete.
   ///
   /// Preconditions:

folly/futures/test/FutureTest.cpp

@@ -219,6 +219,7 @@ TEST(Future, hasPreconditionValid) {
 
   DOIT(f.isReady());
   DOIT(f.result());
+  DOIT(std::move(f).getTry());
   DOIT(std::move(f).get());
   DOIT(std::move(f).get(std::chrono::milliseconds(10)));
   DOIT(f.hasValue());
@@ -304,6 +305,7 @@ TEST(Future, hasPostconditionInvalid) {
   DOIT(makeValid(), swallow(std::move(f).wait()));
   DOIT(makeValid(), swallow(std::move(f.wait())));
   DOIT(makeValid(), swallow(std::move(f).get()));
+  DOIT(makeValid(), swallow(std::move(f).getTry()));
   DOIT(makeValid(), swallow(std::move(f).get(std::chrono::milliseconds(10))));
   DOIT(makeValid(), swallow(std::move(f).semi()));
 
@@ -912,13 +914,13 @@ TEST(Future, futureNotReady) {
 }
 
 TEST(Future, hasException) {
-  EXPECT_TRUE(makeFuture<int>(eggs).result().hasException());
-  EXPECT_FALSE(makeFuture(42).result().hasException());
+  EXPECT_TRUE(makeFuture<int>(eggs).getTry().hasException());
+  EXPECT_FALSE(makeFuture(42).getTry().hasException());
 }
 
 TEST(Future, hasValue) {
-  EXPECT_TRUE(makeFuture(42).result().hasValue());
-  EXPECT_FALSE(makeFuture<int>(eggs).result().hasValue());
+  EXPECT_TRUE(makeFuture(42).getTry().hasValue());
+  EXPECT_FALSE(makeFuture<int>(eggs).getTry().hasValue());
 }
 
 TEST(Future, makeFuture) {
@@ -1473,6 +1475,36 @@ TEST(Future, DetachTest) {
   EXPECT_TRUE(result == 3);
 }
 
+TEST(Future, SimpleGet) {
+  Promise<int> p;
+  auto sf = p.getFuture();
+  p.setValue(3);
+  auto v = std::move(sf).get();
+  ASSERT_EQ(v, 3);
+}
+
+TEST(Future, SimpleGetTry) {
+  Promise<int> p;
+  auto sf = p.getFuture();
+  p.setValue(3);
+  auto v = std::move(sf).getTry();
+  ASSERT_EQ(v.value(), 3);
+}
+
+TEST(Future, SimpleTimedGet) {
+  Promise<folly::Unit> p;
+  auto sf = p.getFuture();
+  EXPECT_THROW(
+      std::move(sf).get(std::chrono::milliseconds(100)), FutureTimeout);
+}
+
+TEST(Future, SimpleTimedGetTry) {
+  Promise<folly::Unit> p;
+  auto sf = p.getFuture();
+  EXPECT_THROW(
+      std::move(sf).getTry(std::chrono::milliseconds(100)), FutureTimeout);
+}
+
 #if FOLLY_FUTURE_USING_FIBER
 
 TEST(Future, BatonWait) {