From 991a1d5e52b35c1ced25818ae7681f0cb69c76ba Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Wed, 21 Jan 2015 11:35:42 -0800 Subject: [PATCH] waitWithSemaphore -> Future::wait() Summary: see task. also adjust all callsites to get() or wait() as appropriate. this is a lot better, especially in various tests Facebook: cc @rushix, @wez as this was quite nice for docstore and novak, and you might want to be aware of new conventions Test Plan: futures unit, wait for contbuild Reviewed By: hans@fb.com Subscribers: trunkagent, fbcode-common-diffs@, hero-diffs@, cold-storage-diffs@, adamsyta, zhuohuang, darshan, micha, folly-diffs@, lins, tingy, hannesr, jsedgwick, wez, rushix FB internal diff: D1785572 Tasks: 5940008 Signature: t1:1785572:1421866794:a879de4d0bc14e96c434f623ed2a74361e25f28c --- folly/futures/Future-inl.h | 87 ++++++++++--------------------- folly/futures/Future.h | 27 ++++------ folly/futures/test/FutureTest.cpp | 35 ++++++------- 3 files changed, 52 insertions(+), 97 deletions(-) diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h index 3b2be169..4eeb084d 100644 --- a/folly/futures/Future-inl.h +++ b/folly/futures/Future-inl.h @@ -643,65 +643,6 @@ whenN(InputIterator first, InputIterator last, size_t n) { return ctx->p.getFuture(); } -template -Future -waitWithSemaphore(Future&& f) { - Baton<> baton; - auto done = f.then([&](Try &&t) { - baton.post(); - return std::move(t.value()); - }); - baton.wait(); - while (!done.isReady()) { - // There's a race here between the return here and the actual finishing of - // the future. f is completed, but the setup may not have finished on done - // after the baton has posted. - std::this_thread::yield(); - } - return done; -} - -template<> -inline Future waitWithSemaphore(Future&& f) { - Baton<> baton; - auto done = f.then([&](Try &&t) { - baton.post(); - t.value(); - }); - baton.wait(); - while (!done.isReady()) { - // There's a race here between the return here and the actual finishing of - // the future. f is completed, but the setup may not have finished on done - // after the baton has posted. - std::this_thread::yield(); - } - return done; -} - -template -Future -waitWithSemaphore(Future&& f, Dur timeout) { - auto baton = std::make_shared>(); - auto done = f.then([baton](Try &&t) { - baton->post(); - return std::move(t.value()); - }); - baton->timed_wait(std::chrono::system_clock::now() + timeout); - return done; -} - -template -Future -waitWithSemaphore(Future&& f, Dur timeout) { - auto baton = std::make_shared>(); - auto done = f.then([baton](Try &&t) { - baton->post(); - t.value(); - }); - baton->timed_wait(std::chrono::system_clock::now() + timeout); - return done; -} - namespace { template void getWaitHelper(Future* f) { @@ -834,6 +775,34 @@ Future Future::delayed(Duration dur, Timekeeper* tk) { }); } +template +Future Future::wait() { + Baton<> baton; + auto done = then([&](Try t) { + baton.post(); + return makeFuture(std::move(t)); + }); + baton.wait(); + while (!done.isReady()) { + // There's a race here between the return here and the actual finishing of + // the future. f is completed, but the setup may not have finished on done + // after the baton has posted. + std::this_thread::yield(); + } + return done; +} + +template +Future Future::wait(Duration dur) { + auto baton = std::make_shared>(); + auto done = then([baton](Try t) { + baton->post(); + return makeFuture(std::move(t)); + }); + baton->timed_wait(std::chrono::system_clock::now() + dur); + return done; +} + } // 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 cd3c36dc..6c015c27 100644 --- a/folly/futures/Future.h +++ b/folly/futures/Future.h @@ -478,6 +478,15 @@ class Future { /// now. The optional Timekeeper is as with futures::sleep(). Future delayed(Duration, Timekeeper* = nullptr); + /// Block until this Future is complete. Returns a new Future containing the + /// result. + Future wait(); + + /// Block until this Future is complete or until the given Duration passes. + /// Returns a new Future which either contains the result or is incomplete, + /// depending on whether the Duration passed. + Future wait(Duration); + private: typedef detail::Core* corePtr; @@ -608,24 +617,6 @@ Future::value_type::value_type>>>> whenN(InputIterator first, InputIterator last, size_t n); -/** Wait for the given future to complete on a semaphore. Returns a completed - * future containing the result. - * - * NB if the promise for the future would be fulfilled in the same thread that - * you call this, it will deadlock. - */ -template -Future waitWithSemaphore(Future&& f); - -/** Wait for up to `timeout` for the given future to complete. Returns a future - * which may or may not be completed depending whether the given future - * completed in time - * - * Note: each call to this starts a (short-lived) thread and allocates memory. - */ -template -Future waitWithSemaphore(Future&& f, Dur timeout); - } // folly #include diff --git a/folly/futures/test/FutureTest.cpp b/folly/futures/test/FutureTest.cpp index dbe0b942..3b7763f9 100644 --- a/folly/futures/test/FutureTest.cpp +++ b/folly/futures/test/FutureTest.cpp @@ -873,31 +873,31 @@ TEST(Future, throwIfFailed) { }); } -TEST(Future, waitWithSemaphoreImmediate) { - waitWithSemaphore(makeFuture()); - auto done = waitWithSemaphore(makeFuture(42)).value(); +TEST(Future, waitImmediate) { + makeFuture().wait(); + auto done = makeFuture(42).wait().value(); EXPECT_EQ(42, done); vector v{1,2,3}; - auto done_v = waitWithSemaphore(makeFuture(v)).value(); + auto done_v = makeFuture(v).wait().value(); EXPECT_EQ(v.size(), done_v.size()); EXPECT_EQ(v, done_v); vector> v_f; v_f.push_back(makeFuture()); v_f.push_back(makeFuture()); - auto done_v_f = waitWithSemaphore(whenAll(v_f.begin(), v_f.end())).value(); + auto done_v_f = whenAll(v_f.begin(), v_f.end()).wait().value(); EXPECT_EQ(2, done_v_f.size()); vector> v_fb; v_fb.push_back(makeFuture(true)); v_fb.push_back(makeFuture(false)); auto fut = whenAll(v_fb.begin(), v_fb.end()); - auto done_v_fb = std::move(waitWithSemaphore(std::move(fut)).value()); + auto done_v_fb = std::move(fut.wait().value()); EXPECT_EQ(2, done_v_fb.size()); } -TEST(Future, waitWithSemaphore) { +TEST(Future, wait) { Promise p; Future f = p.getFuture(); std::atomic flag{false}; @@ -910,7 +910,7 @@ TEST(Future, waitWithSemaphore) { return t.value(); }); flag = true; - result.store(waitWithSemaphore(std::move(n)).value()); + result.store(n.wait().value()); }, std::move(f) ); @@ -924,12 +924,11 @@ TEST(Future, waitWithSemaphore) { EXPECT_EQ(result.load(), 42); } -TEST(Future, waitWithSemaphoreForTime) { +TEST(Future, waitWithDuration) { { Promise p; Future f = p.getFuture(); - auto t = waitWithSemaphore(std::move(f), - std::chrono::microseconds(1)); + auto t = f.wait(std::chrono::milliseconds(1)); EXPECT_FALSE(t.isReady()); p.setValue(1); EXPECT_TRUE(t.isReady()); @@ -938,8 +937,7 @@ TEST(Future, waitWithSemaphoreForTime) { Promise p; Future f = p.getFuture(); p.setValue(1); - auto t = waitWithSemaphore(std::move(f), - std::chrono::milliseconds(1)); + auto t = f.wait(std::chrono::milliseconds(1)); EXPECT_TRUE(t.isReady()); } { @@ -947,8 +945,7 @@ TEST(Future, waitWithSemaphoreForTime) { v_fb.push_back(makeFuture(true)); v_fb.push_back(makeFuture(false)); auto f = whenAll(v_fb.begin(), v_fb.end()); - auto t = waitWithSemaphore(std::move(f), - std::chrono::milliseconds(1)); + auto t = f.wait(std::chrono::milliseconds(1)); EXPECT_TRUE(t.isReady()); EXPECT_EQ(2, t.value().size()); } @@ -959,8 +956,7 @@ TEST(Future, waitWithSemaphoreForTime) { v_fb.push_back(p1.getFuture()); v_fb.push_back(p2.getFuture()); auto f = whenAll(v_fb.begin(), v_fb.end()); - auto t = waitWithSemaphore(std::move(f), - std::chrono::milliseconds(1)); + auto t = f.wait(std::chrono::milliseconds(1)); EXPECT_FALSE(t.isReady()); p1.setValue(true); EXPECT_FALSE(t.isReady()); @@ -968,8 +964,7 @@ TEST(Future, waitWithSemaphoreForTime) { EXPECT_TRUE(t.isReady()); } { - auto t = waitWithSemaphore(makeFuture(), - std::chrono::milliseconds(1)); + auto t = makeFuture().wait(std::chrono::milliseconds(1)); EXPECT_TRUE(t.isReady()); } } @@ -1171,7 +1166,7 @@ TEST(Future, t5506504) { return whenAll(futures.begin(), futures.end()); }; - waitWithSemaphore(fn()); + fn().wait(); } // Test of handling of a circular dependency. It's never recommended -- 2.34.1