Make folly::TimeKeeper::after and ::at return SemiFuture

Summary:
TimeKeeper::after and TimeKeeper::at return Futures that for the majority of implementations complete on a TimeKeeper owned executor. This can lead to problems.

Consistent with other API cleanups, this diff switches TimeKeeper APIs to return SemiFuture, requiring that the caller attach an explicit executor when work is to be scheduled downstream of the TimeKeeper.

Reviewed By: yfeldblum

Differential Revision: D17005944

fbshipit-source-id: 98d7e8aaf1ad40ce2180503f632f2f0003bb12be

folly/executors/test/TimekeeperScheduledExecutorTest.cpp

@@ -32,6 +32,7 @@ using folly::Future;
 using folly::ManualTimekeeper;
 using folly::Promise;
 using folly::ScheduledExecutor;
+using folly::SemiFuture;
 using folly::TimekeeperScheduledExecutor;
 using folly::Unit;
 using std::chrono::steady_clock;
@@ -39,8 +40,7 @@ using time_point = steady_clock::time_point;
 
 namespace {
 void simpleTest(std::unique_ptr<folly::Executor> const& parent) {
-  auto tk = std::make_shared<ManualTimekeeper>(
-      folly::getKeepAliveToken(parent.get()));
+  auto tk = std::make_shared<ManualTimekeeper>();
   auto executor = TimekeeperScheduledExecutor::create(
       folly::getKeepAliveToken(parent.get()), [tk]() { return tk; });
 

folly/futures/Future-inl.h

@@ -2017,8 +2017,8 @@ SemiFuture<T> SemiFuture<T>::within(Duration dur, E e, Timekeeper* tk) && {
 
   // Have time keeper use a weak ptr to hold ctx,
   // so that ctx can be deallocated as soon as the future job finished.
-  ctx->afterFuture = tk->after(dur).semi().defer(
-      [weakCtx = to_weak_ptr(ctx)](Try<Unit>&& t) mutable {
+  ctx->afterFuture =
+      tk->after(dur).defer([weakCtx = to_weak_ptr(ctx)](Try<Unit>&& t) mutable {
         if (t.hasException() &&
             t.exception().is_compatible_with<FutureCancellation>()) {
           // This got cancelled by thisFuture so we can just return.
@@ -2514,11 +2514,11 @@ auto ensure(F&& f, Ensure&& ensure) {
 } // namespace futures
 
 template <class Clock>
-Future<Unit> Timekeeper::at(std::chrono::time_point<Clock> when) {
+SemiFuture<Unit> Timekeeper::at(std::chrono::time_point<Clock> when) {
   auto now = Clock::now();
 
   if (when <= now) {
-    return makeFuture();
+    return makeSemiFuture();
   }
 
   return after(std::chrono::duration_cast<Duration>(when - now));

folly/futures/Future.h

@@ -1922,36 +1922,38 @@ class Timekeeper {
   /// exceptional. Use the steady (monotonic) clock.
   ///
   /// The consumer thread may cancel this Future to reclaim resources.
-  ///
-  /// This future probably completes on the timer thread. You should almost
-  /// certainly follow it with a via() call or the accuracy of other timers
-  /// will suffer.
-  virtual Future<Unit> after(Duration dur) = 0;
+  virtual SemiFuture<Unit> after(Duration dur) = 0;
 
   /// Unsafe version of after that returns an inline Future.
   /// Any work added to this future will run inline on the Timekeeper's thread.
   /// This can potentially cause problems with timing.
+  ///
+  /// Please migrate to use after + a call to via with a valid, non-inline
+  /// executor.
   Future<Unit> afterUnsafe(Duration dur) {
-    return after(dur).semi().toUnsafeFuture();
+    return after(dur).toUnsafeFuture();
   }
 
   /// Returns a future that will complete at the requested time.
   ///
-  /// You may cancel this Future to reclaim resources.
+  /// You may cancel this SemiFuture to reclaim resources.
   ///
   /// NB This is sugar for `after(when - now)`, so while you are welcome to
   /// use a std::chrono::system_clock::time_point it will not track changes to
   /// the system clock but rather execute that many milliseconds in the future
   /// according to the steady clock.
   template <class Clock>
-  Future<Unit> at(std::chrono::time_point<Clock> when);
+  SemiFuture<Unit> at(std::chrono::time_point<Clock> when);
 
   /// Unsafe version of at that returns an inline Future.
   /// Any work added to this future will run inline on the Timekeeper's thread.
   /// This can potentially cause problems with timing.
+  ///
+  /// Please migrate to use at + a call to via with a valid, non-inline
+  /// executor.
   template <class Clock>
   Future<Unit> atUnsafe(std::chrono::time_point<Clock> when) {
-    return at(when).semi().toUnsafeFuture();
+    return at(when).toUnsafeFuture();
   }
 };
 

folly/futures/ManualTimekeeper.cpp

@@ -17,11 +17,10 @@
 
 namespace folly {
 
-ManualTimekeeper::ManualTimekeeper(Executor::KeepAlive<Executor>&& executor)
-    : executor_{std::move(executor)}, now_{std::chrono::steady_clock::now()} {}
+ManualTimekeeper::ManualTimekeeper() : now_{std::chrono::steady_clock::now()} {}
 
-Future<Unit> ManualTimekeeper::after(Duration dur) {
-  auto contract = folly::makePromiseContract<Unit>(executor_.get());
+SemiFuture<Unit> ManualTimekeeper::after(Duration dur) {
+  auto contract = folly::makePromiseContract<Unit>();
   if (dur.count() == 0) {
     contract.first.setValue(folly::unit);
   } else {

folly/futures/ManualTimekeeper.h

@@ -30,11 +30,11 @@ namespace folly {
 // thread, while after() can safely be called from multiple threads.
 class ManualTimekeeper : public folly::Timekeeper {
  public:
-  explicit ManualTimekeeper(Executor::KeepAlive<Executor>&& executor);
+  explicit ManualTimekeeper();
 
   /// The returned future is completed when someone calls advance and pushes the
   /// executor's clock to a value greater than or equal to (now() + dur)
-  Future<Unit> after(folly::Duration dur) override;
+  SemiFuture<Unit> after(folly::Duration dur) override;
 
   /// Advance the timekeeper's clock to (now() + dur).  All futures with target
   /// time points less than or equal to (now() + dur) are fulfilled after the

folly/futures/ThreadWheelTimekeeper.cpp

@@ -44,8 +44,8 @@ struct WTCallback : public std::enable_shared_from_this<WTCallback>,
     return cob;
   }
 
-  Future<Unit> getFuture() {
-    return promise_.getFuture();
+  SemiFuture<Unit> getSemiFuture() {
+    return promise_.getSemiFuture();
   }
 
   FOLLY_NODISCARD Promise<Unit> stealPromise() {
@@ -120,9 +120,9 @@ ThreadWheelTimekeeper::~ThreadWheelTimekeeper() {
   thread_.join();
 }
 
-Future<Unit> ThreadWheelTimekeeper::after(Duration dur) {
+SemiFuture<Unit> ThreadWheelTimekeeper::after(Duration dur) {
   auto cob = WTCallback::create(&eventBase_);
-  auto f = cob->getFuture();
+  auto f = cob->getSemiFuture();
   //
   // Even shared_ptr of cob is captured in lambda this is still somewhat *racy*
   // because it will be released once timeout is scheduled. So technically there

folly/futures/ThreadWheelTimekeeper.h

@@ -33,10 +33,7 @@ class ThreadWheelTimekeeper : public Timekeeper {
   ~ThreadWheelTimekeeper() override;
 
   /// Implement the Timekeeper interface
-  /// This future *does* complete on the timer thread. You should almost
-  /// certainly follow it with a via() call or the accuracy of other timers
-  /// will suffer.
-  Future<Unit> after(Duration) override;
+  SemiFuture<Unit> after(Duration) override;
 
  protected:
   folly::EventBase eventBase_;

folly/futures/test/ManualTimekeeperTest.cpp

@@ -27,14 +27,14 @@ namespace folly {
 class ManualTimekeeperTest : public ::testing::Test {};
 
 TEST_F(ManualTimekeeperTest, Basic) {
-  auto timekeeper = folly::ManualTimekeeper{&folly::InlineExecutor::instance()};
+  auto timekeeper = folly::ManualTimekeeper{};
   auto future = timekeeper.after(100s);
   timekeeper.advance(100s);
   EXPECT_TRUE(future.isReady());
 }
 
 TEST_F(ManualTimekeeperTest, AdvanceWithoutAnyFutures) {
-  auto timekeeper = folly::ManualTimekeeper{&folly::InlineExecutor::instance()};
+  auto timekeeper = folly::ManualTimekeeper{};
   timekeeper.advance(100s);
   auto future = timekeeper.after(100s);
   EXPECT_FALSE(future.isReady());
@@ -43,7 +43,7 @@ TEST_F(ManualTimekeeperTest, AdvanceWithoutAnyFutures) {
 }
 
 TEST_F(ManualTimekeeperTest, AdvanceWithManyFutures) {
-  auto timekeeper = folly::ManualTimekeeper{&folly::InlineExecutor::instance()};
+  auto timekeeper = folly::ManualTimekeeper{};
 
   auto one = timekeeper.after(100s);
   auto two = timekeeper.after(200s);

folly/futures/test/TimekeeperTest.cpp

@@ -136,8 +136,8 @@ TEST(Timekeeper, semiFutureWithinCancelsTimeout) {
       });
     }
 
-    Future<Unit> after(Duration) override {
-      return p_.getFuture();
+    SemiFuture<Unit> after(Duration) override {
+      return p_.getSemiFuture();
     }
 
     Promise<Unit> p_;
@@ -155,8 +155,8 @@ TEST(Timekeeper, semiFutureWithinCancelsTimeout) {
 
 TEST(Timekeeper, semiFutureWithinInlineAfter) {
   struct MockTimekeeper : Timekeeper {
-    Future<Unit> after(Duration) override {
-      return folly::makeFuture<folly::Unit>(folly::FutureNoTimekeeper());
+    SemiFuture<Unit> after(Duration) override {
+      return folly::makeSemiFuture<folly::Unit>(folly::FutureNoTimekeeper());
     }
   };
 
@@ -169,9 +169,9 @@ TEST(Timekeeper, semiFutureWithinInlineAfter) {
 
 TEST(Timekeeper, semiFutureWithinReady) {
   struct MockTimekeeper : Timekeeper {
-    Future<Unit> after(Duration) override {
+    SemiFuture<Unit> after(Duration) override {
       called_ = true;
-      return folly::makeFuture<folly::Unit>(folly::FutureNoTimekeeper());
+      return folly::makeSemiFuture<folly::Unit>(folly::FutureNoTimekeeper());
     }
 
     bool called_{false};