From 9f916e143c9efad2f24cf6b62c90c8ac7895f71b Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Wed, 21 Jan 2015 13:39:03 -0800 Subject: [PATCH] getVia() and waitVia() Summary: Introduce ProgressableExecutor, which is an Executor that can be driven somehow. Examples include EventBase and ManualExecutor Then introduce Future::getVia(ProgressableExecutor*) and Future::waitVia(ProgressableExecutor*) that drive the given executor until the Future is complete, with the usual semantics of get and wait respectively This is a really common pattern in tests and you can see in the various changes to other projects lends sopme nice redness and cleanliness Some notes: 1. I don't like ProgressableExecutor::makeProgress() that much. Too verbose. Maybe DrivableExecutor::drive()? Something else? Thoughts? 2. I still need to integrate this with some stuff in Zeus (SessionFuture) and Zookeeper (ZookeeperFuture) but I'm going to do that in a separate diff because it's going to be a little more intrusive 3. These APIs take a raw ptr so that they are consistent with via() 4. See inline note on ManualExecutor 5. See inline note in added unit tests Test Plan: add unit for new API, wait for contbuild Reviewed By: hans@fb.com Subscribers: trunkagent, dresende, pzq, tdimson, fbcode-common-diffs@, targeting-diff-backend@, alandau, apollo-diffs@, bmatheny, everstore-dev@, zhuohuang, laser-diffs@, mshneer, folly-diffs@, hannesr, jsedgwick FB internal diff: D1789122 Tasks: 5940008 Signature: t1:1789122:1421868315:6ea2fc2702be1dc283c24a46d345fb5da3935b32 --- folly/Makefile.am | 1 + folly/futures/DrivableExecutor.h | 47 +++++++++++++++++++++++++++ folly/futures/Future-inl.h | 32 ++++++++++++++++++ folly/futures/Future.h | 16 ++++++++- folly/futures/ManualExecutor.h | 9 +++++- folly/futures/ScheduledExecutor.h | 2 +- folly/futures/test/FutureTest.cpp | 54 +++++++++++++++++++++++++++++++ folly/futures/test/ViaTest.cpp | 21 ++++++------ folly/io/async/EventBase.h | 12 +++++-- 9 files changed, 177 insertions(+), 17 deletions(-) create mode 100644 folly/futures/DrivableExecutor.h diff --git a/folly/Makefile.am b/folly/Makefile.am index ce7f1e50..223206f0 100644 --- a/folly/Makefile.am +++ b/folly/Makefile.am @@ -85,6 +85,7 @@ nobase_follyinclude_HEADERS = \ Format.h \ Format-inl.h \ futures/Deprecated.h \ + futures/DrivableExecutor.h \ futures/Future-inl.h \ futures/Future.h \ futures/FutureException.h \ diff --git a/folly/futures/DrivableExecutor.h b/folly/futures/DrivableExecutor.h new file mode 100644 index 00000000..57f177c6 --- /dev/null +++ b/folly/futures/DrivableExecutor.h @@ -0,0 +1,47 @@ +/* + * Copyright 2015 Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +namespace folly { + +/* + * A DrivableExecutor can be driven via its drive() method + * Examples include EventBase (via loopOnce()) and ManualExecutor + * (via makeProgress()). + * + * This interface is most handy in conjunction with + * Future::getVia(DrivableExecutor*) and + * Future::waitVia(DrivableExecutor*) + * + * These call drive() * repeatedly until the Future is fulfilled. + * getVia() returns the value (or throws the exception) and waitVia() returns + * the same Future for chainability. + * + * These will be most helpful in tests, for instance if you need to pump a mock + * EventBase until Futures complete. + */ +class DrivableExecutor : public virtual Executor { + public: + virtual ~DrivableExecutor() = default; + + // Make progress on this Executor's work. + virtual void drive() = 0; +}; + +} // folly diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h index 4eeb084d..34940ced 100644 --- a/folly/futures/Future-inl.h +++ b/folly/futures/Future-inl.h @@ -720,6 +720,22 @@ inline void Future::get(Duration dur) { getWaitTimeoutHelper(this, dur).value(); } +template +T Future::getVia(DrivableExecutor* e) { + while (!isReady()) { + e->drive(); + } + return std::move(value()); +} + +template <> +inline void Future::getVia(DrivableExecutor* e) { + while (!isReady()) { + e->drive(); + } + value(); +} + template Future Future::within(Duration dur, Timekeeper* tk) { return within(dur, TimedOut(), tk); @@ -803,6 +819,22 @@ Future Future::wait(Duration dur) { return done; } +template +Future& Future::waitVia(DrivableExecutor* e) & { + while (!isReady()) { + e->drive(); + } + return *this; +} + +template +Future Future::waitVia(DrivableExecutor* e) && { + while (!isReady()) { + e->drive(); + } + return std::move(*this); +} + } // I haven't included a Future specialization because I don't forsee us diff --git a/folly/futures/Future.h b/folly/futures/Future.h index 6c015c27..7d210a76 100644 --- a/folly/futures/Future.h +++ b/folly/futures/Future.h @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -90,7 +91,6 @@ struct Extract { typedef typename ArgType::FirstArg FirstArg; }; - } // detail struct Timekeeper; @@ -190,6 +190,11 @@ class Future { /// exception). T get(Duration dur); + /// Call e->drive() repeatedly until the future is fulfilled. Examples + /// of DrivableExecutor include EventBase and ManualExecutor. Returns the + /// value (moved out), or throws the exception. + T getVia(DrivableExecutor* e); + /** When this Future has completed, execute func which is a function that takes a Try&&. A Future for the return type of func is returned. e.g. @@ -487,6 +492,15 @@ class Future { /// depending on whether the Duration passed. Future wait(Duration); + /// Call e->drive() repeatedly until the future is fulfilled. Examples + /// of DrivableExecutor include EventBase and ManualExecutor. Returns a + /// reference to this Future so that you can chain calls if desired. + /// value (moved out), or throws the exception. + Future& waitVia(DrivableExecutor* e) &; + + /// Overload of waitVia() for rvalue Futures + Future waitVia(DrivableExecutor* e) &&; + private: typedef detail::Core* corePtr; diff --git a/folly/futures/ManualExecutor.h b/folly/futures/ManualExecutor.h index db6a17da..e1e38cd9 100644 --- a/folly/futures/ManualExecutor.h +++ b/folly/futures/ManualExecutor.h @@ -15,6 +15,7 @@ */ #pragma once +#include #include #include #include @@ -31,7 +32,8 @@ namespace folly { /// /// NB No attempt has been made to make anything other than add and schedule /// threadsafe. - class ManualExecutor : public ScheduledExecutor { + class ManualExecutor : public DrivableExecutor, + public ScheduledExecutor { public: ManualExecutor(); @@ -54,6 +56,11 @@ namespace folly { run(); } + /// Implements DrivableExecutor + void drive() override { + makeProgress(); + } + /// makeProgress until this Future is ready. template void waitFor(F const& f) { // TODO(5427828) diff --git a/folly/futures/ScheduledExecutor.h b/folly/futures/ScheduledExecutor.h index a3f6dedb..5d15e84c 100644 --- a/folly/futures/ScheduledExecutor.h +++ b/folly/futures/ScheduledExecutor.h @@ -23,7 +23,7 @@ namespace folly { // An executor that supports timed scheduling. Like RxScheduler. - class ScheduledExecutor : public Executor { + class ScheduledExecutor : public virtual Executor { public: // Reality is that better than millisecond resolution is very hard to // achieve. However, we reserve the right to be incredible. diff --git a/folly/futures/test/FutureTest.cpp b/folly/futures/test/FutureTest.cpp index 3b7763f9..dad82fd3 100644 --- a/folly/futures/test/FutureTest.cpp +++ b/folly/futures/test/FutureTest.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -969,6 +970,59 @@ TEST(Future, waitWithDuration) { } } +class DummyDrivableExecutor : public DrivableExecutor { + public: + void add(Func f) override {} + void drive() override { ran = true; } + bool ran{false}; +}; + +TEST(Future, getVia) { + { + // non-void + ManualExecutor x; + auto f = via(&x).then([]{ return true; }); + EXPECT_TRUE(f.getVia(&x)); + } + + { + // void + ManualExecutor x; + auto f = via(&x).then(); + f.getVia(&x); + } + + { + DummyDrivableExecutor x; + auto f = makeFuture(true); + EXPECT_TRUE(f.getVia(&x)); + EXPECT_FALSE(x.ran); + } +} + +TEST(Future, waitVia) { + { + ManualExecutor x; + auto f = via(&x).then(); + EXPECT_FALSE(f.isReady()); + f.waitVia(&x); + EXPECT_TRUE(f.isReady()); + } + + { + // try rvalue as well + ManualExecutor x; + auto f = via(&x).activate().then().waitVia(&x); + EXPECT_TRUE(f.isReady()); + } + + { + DummyDrivableExecutor x; + makeFuture(true).waitVia(&x); + EXPECT_FALSE(x.ran); + } +} + TEST(Future, callbackAfterActivate) { Promise p; auto f = p.getFuture(); diff --git a/folly/futures/test/ViaTest.cpp b/folly/futures/test/ViaTest.cpp index 2bbd89f0..5a76f3c1 100644 --- a/folly/futures/test/ViaTest.cpp +++ b/folly/futures/test/ViaTest.cpp @@ -20,13 +20,18 @@ #include #include #include +#include using namespace folly; -struct ManualWaiter { +struct ManualWaiter : public DrivableExecutor { explicit ManualWaiter(std::shared_ptr ex) : ex(ex) {} - void makeProgress() { + void add(Func f) { + ex->add(f); + } + + void drive() override { ex->wait(); ex->run(); } @@ -44,7 +49,7 @@ struct ViaFixture : public testing::Test { t = std::thread([=] { ManualWaiter eastWaiter(eastExecutor); while (!done) - eastWaiter.makeProgress(); + eastWaiter.drive(); }); } @@ -146,10 +151,7 @@ TEST_F(ViaFixture, thread_hops) { EXPECT_EQ(std::this_thread::get_id(), westThreadId); return t.value(); }); - while (!f.isReady()) { - waiter->makeProgress(); - } - EXPECT_EQ(f.value(), 1); + EXPECT_EQ(f.getVia(waiter.get()), 1); } TEST_F(ViaFixture, chain_vias) { @@ -169,10 +171,7 @@ TEST_F(ViaFixture, chain_vias) { return t.value(); }); - while (!f.isReady()) { - waiter->makeProgress(); - } - EXPECT_EQ(f.value(), 1); + EXPECT_EQ(f.getVia(waiter.get()), 1); } TEST_F(ViaFixture, bareViaAssignment) { diff --git a/folly/io/async/EventBase.h b/folly/io/async/EventBase.h index 109da17c..484977b1 100644 --- a/folly/io/async/EventBase.h +++ b/folly/io/async/EventBase.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -95,9 +96,9 @@ class RequestEventBase : public RequestData { * EventBase from other threads. When it is safe to call a method from * another thread it is explicitly listed in the method comments. */ -class EventBase : - private boost::noncopyable, public TimeoutManager, public Executor -{ +class EventBase : private boost::noncopyable, + public TimeoutManager, + public DrivableExecutor { public: /** * A callback interface to use with runInLoop() @@ -485,6 +486,11 @@ class EventBase : runInEventBaseThread(fn); } + /// Implements the DrivableExecutor interface + void drive() override { + loopOnce(); + } + private: // TimeoutManager -- 2.34.1