SemiFuture wait() and timed_wait() without TimedDrivableExecutor

Reviewed By: yfeldblum

Differential Revision: D6893567

fbshipit-source-id: 054e109e3c3c3b2b20ac6ffe8f2ee05335e3dd89

folly/futures/Future-pre.h

@@ -165,141 +165,7 @@ struct Extract<R (&)(Args...)> {
   typedef typename ArgType<Args...>::FirstArg FirstArg;
 };
 
-/**
- * Defer work until executor is chained.
- *
- * NOTE: that this executor is a private implementation detail belonging to the
- * Folly Futures library and not intended to be used elsewhere. It is designed
- * specifically for the use case of deferring work on a SemiFuture. It is NOT
- * thread safe. Please do not use for any other purpose without great care.
- */
-class DeferredExecutor final : public Executor {
- public:
-  template <typename Class, typename F>
-  struct DeferredWorkWrapper;
-
-  /**
-   * Work wrapper class to capture the keepalive and forward the argument
-   * list to the captured function.
-   */
-  template <typename F, typename R, typename... Args>
-  struct DeferredWorkWrapper<F, R (F::*)(Args...) const> {
-    R operator()(Args... args) {
-      return func(std::forward<Args>(args)...);
-    }
-
-    Executor::KeepAlive a;
-    F func;
-  };
-
-  /**
-   * Construction is private to ensure that creation and deletion are
-   * symmetric
-   */
-  static KeepAlive create() {
-    std::unique_ptr<futures::detail::DeferredExecutor> devb{
-        new futures::detail::DeferredExecutor{}};
-    auto keepAlive = devb->getKeepAliveToken();
-    devb.release();
-    return keepAlive;
-  }
-
-  /// Enqueue a function to executed by this executor. This is not thread-safe.
-  void add(Func func) override {
-    // We should never have a function here already. Either we are RUNNING,
-    // in which case we are on one and it should have been removed from the
-    // executor, or we are not in which case it should be the first.
-    assert(!func_);
-
-    // If we are already running, must be reentrant. Just call func.
-    if (state_.load() == State::RUNNING) {
-      func();
-      return;
-    }
-
-    // If we already have a function, wrap and chain. Otherwise assign.
-    func_ = std::move(func);
-
-    State expected = State::NEW;
-    // If the state is new, then attempt to change it to HAS_CALLBACK, set the
-    // executor and return.
-    if (state_.load() == expected) {
-      if (state_.compare_exchange_strong(expected, State::HAS_CALLBACK)) {
-        return;
-      }
-    }
-    // If we have the executor set, we now have the callback too.
-    // Enqueue the callback on the executor and change to the RUNNING state.
-    enqueueWork();
-  }
-
-  void setExecutor(Executor* exec) {
-    executorKeepAlive_ = exec->getKeepAliveToken();
-    State expected = State::NEW;
-    // If the state is new, then attempt to change it to HAS_EXECUTOR, set the
-    // executor and return.
-    if (state_.load() == expected) {
-      if (state_.compare_exchange_strong(expected, State::HAS_EXECUTOR)) {
-        return;
-      }
-    }
-    // If we have the callback set, we now have the executor too.
-    // Enqueue the callback on the executor and change to the RUNNING state.
-    enqueueWork();
-  }
-
-  KeepAlive getKeepAliveToken() override {
-    keepAliveAcquire();
-    return makeKeepAlive();
-  }
-
-  ~DeferredExecutor() = default;
-
-  template <class F>
-  static auto wrap(Executor::KeepAlive keepAlive, F&& func)
-      -> DeferredWorkWrapper<F, decltype(&F::operator())> {
-    return DeferredExecutor::DeferredWorkWrapper<F, decltype(&F::operator())>{
-        std::move(keepAlive), std::forward<F>(func)};
-  }
-
- protected:
-  void keepAliveAcquire() override {
-    ++keepAliveCount_;
-  }
-
-  void keepAliveRelease() override {
-    releaseAndTryFree();
-  }
-
-  void releaseAndTryFree() {
-    --keepAliveCount_;
-    if (keepAliveCount_ == 0) {
-      delete this;
-    }
-  }
-
- private:
-  enum class State {
-    NEW,
-    HAS_EXECUTOR,
-    HAS_CALLBACK,
-    RUNNING,
-  };
-
-  Func func_;
-  ssize_t keepAliveCount_{0};
-  std::atomic<State> state_{State::NEW};
-  KeepAlive executorKeepAlive_;
-
-  DeferredExecutor() = default;
-
-  void enqueueWork() {
-    DCHECK(func_);
-    state_.store(State::RUNNING);
-    executorKeepAlive_.get()->add(std::move(func_));
-    return;
-  }
-};
+class DeferredExecutor;
 
 } // namespace detail
 } // namespace futures

folly/futures/Future.cpp

@@ -53,52 +53,5 @@ Future<Unit> sleep(Duration dur, Timekeeper* tk) {
   return tk->after(dur);
 }
 
-namespace detail {
-
-struct TimedDrivableExecutorWrapperTag {};
-struct TimedDrivableExecutorWrapperMaker {
-  std::pair<folly::Executor::KeepAlive, TimedDrivableExecutor*> operator()() {
-    std::unique_ptr<futures::detail::TimedDrivableExecutorWrapper> devb{
-        new futures::detail::TimedDrivableExecutorWrapper{}};
-    std::pair<folly::Executor::KeepAlive, TimedDrivableExecutor*> ret{
-        devb->getKeepAliveToken(),
-        static_cast<TimedDrivableExecutor*>(devb.get())};
-    devb.release();
-    return ret;
-  }
-};
-
-std::pair<folly::Executor::KeepAlive, TimedDrivableExecutor*>
-TimedDrivableExecutorWrapper::get() {
-  // Thread-local is a pair of a KeepAlive that owns the executor safely
-  // relative to later created keepalives, and a pre-cast pointer to avoid
-  // recasting later (which would have to be dynamic from Executor*).
-  auto& p = folly::SingletonThreadLocal<
-      std::pair<folly::Executor::KeepAlive, TimedDrivableExecutor*>,
-      TimedDrivableExecutorWrapperTag,
-      TimedDrivableExecutorWrapperMaker>::get();
-  // Reconstruct pair from a new keepalive token and the pointer because
-  // the keepalive is not copyable
-  return {p.second->getKeepAliveToken(), p.second};
-}
-
-void TimedDrivableExecutorWrapper::keepAliveAcquire() {
-  keepAliveCount_.fetch_add(1, std::memory_order_relaxed);
-}
-
-void TimedDrivableExecutorWrapper::keepAliveRelease() {
-  if (keepAliveCount_.fetch_sub(1, std::memory_order_release) == 1) {
-    std::atomic_thread_fence(std::memory_order_acquire);
-    delete this;
-  }
-}
-
-folly::Executor::KeepAlive TimedDrivableExecutorWrapper::getKeepAliveToken() {
-  keepAliveAcquire();
-  return makeKeepAlive();
-}
-
-} // namespace detail
-
 } // namespace futures
 } // namespace folly

folly/futures/Future.h

@@ -204,6 +204,8 @@ class SemiFuture : private futures::detail::FutureBase<T> {
   using TimePoint = std::chrono::system_clock::time_point;
 
  public:
+  ~SemiFuture();
+
   static SemiFuture<T> makeEmpty(); // equivalent to moved-from
 
   // Export public interface of FutureBase
@@ -337,6 +339,10 @@ class SemiFuture : private futures::detail::FutureBase<T> {
 
   explicit SemiFuture(futures::detail::EmptyConstruct) noexcept
       : Base(futures::detail::EmptyConstruct{}) {}
+
+  DeferredExecutor* getDeferredExecutor() const;
+
+  static void releaseDeferredExecutor(corePtr core);
 };
 
 template <class T>

folly/futures/test/SemiFutureTest.cpp

@@ -294,10 +294,6 @@ TEST(SemiFuture, SimpleTimedWait) {
   sf.wait(std::chrono::milliseconds(100));
   EXPECT_FALSE(sf.isReady());
   p.setValue();
-  EXPECT_FALSE(sf.isReady());
-  // The internals of wait mean that there is an executor in the way. We
-  // cannot expect that the promise immediately statisfies the future.
-  sf.wait(std::chrono::milliseconds(100));
   EXPECT_TRUE(sf.isReady());
 }