From 47768bc72a4083ca5400665cba3dceeeb937c71b Mon Sep 17 00:00:00 2001 From: Dave Watson Date: Thu, 22 Jan 2015 12:32:26 -0800 Subject: [PATCH] Revert "[futures] waitWithSemaphore -> Future::wait()" Summary: Subject: This reverts commit d8eeb9a12f932b74d268c74ce7ce2610dc14b152. Test Plan: Watch contbuild? Reviewed By: njormrod@fb.com Subscribers: trunkagent, doug, fbcode-common-diffs@, hero-diffs@, cold-storage-diffs@, adamsyta, zhuohuang, darshan, micha, folly-diffs@, lins, tingy, hannesr, jsedgwick FB internal diff: D1797163 Tasks: 5940008 Signature: t1:1797163:1421959040:c9d528367fed2de2f7dafdac9416aa01bdb47df9 --- folly/futures/Future-inl.h | 59 +++++++++++++++++++++++++++++++ folly/futures/Future.h | 18 ++++++++++ folly/futures/test/FutureTest.cpp | 35 ++++++++++-------- 3 files changed, 97 insertions(+), 15 deletions(-) diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h index 34940ced..4b12706f 100644 --- a/folly/futures/Future-inl.h +++ b/folly/futures/Future-inl.h @@ -643,6 +643,65 @@ 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) { diff --git a/folly/futures/Future.h b/folly/futures/Future.h index 7d210a76..948e263a 100644 --- a/folly/futures/Future.h +++ b/folly/futures/Future.h @@ -631,6 +631,24 @@ 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 dad82fd3..a69c8b96 100644 --- a/folly/futures/test/FutureTest.cpp +++ b/folly/futures/test/FutureTest.cpp @@ -874,31 +874,31 @@ TEST(Future, throwIfFailed) { }); } -TEST(Future, waitImmediate) { - makeFuture().wait(); - auto done = makeFuture(42).wait().value(); +TEST(Future, waitWithSemaphoreImmediate) { + waitWithSemaphore(makeFuture()); + auto done = waitWithSemaphore(makeFuture(42)).value(); EXPECT_EQ(42, done); vector v{1,2,3}; - auto done_v = makeFuture(v).wait().value(); + auto done_v = waitWithSemaphore(makeFuture(v)).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 = whenAll(v_f.begin(), v_f.end()).wait().value(); + auto done_v_f = waitWithSemaphore(whenAll(v_f.begin(), v_f.end())).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(fut.wait().value()); + auto done_v_fb = std::move(waitWithSemaphore(std::move(fut)).value()); EXPECT_EQ(2, done_v_fb.size()); } -TEST(Future, wait) { +TEST(Future, waitWithSemaphore) { Promise p; Future f = p.getFuture(); std::atomic flag{false}; @@ -911,7 +911,7 @@ TEST(Future, wait) { return t.value(); }); flag = true; - result.store(n.wait().value()); + result.store(waitWithSemaphore(std::move(n)).value()); }, std::move(f) ); @@ -925,11 +925,12 @@ TEST(Future, wait) { EXPECT_EQ(result.load(), 42); } -TEST(Future, waitWithDuration) { +TEST(Future, waitWithSemaphoreForTime) { { Promise p; Future f = p.getFuture(); - auto t = f.wait(std::chrono::milliseconds(1)); + auto t = waitWithSemaphore(std::move(f), + std::chrono::microseconds(1)); EXPECT_FALSE(t.isReady()); p.setValue(1); EXPECT_TRUE(t.isReady()); @@ -938,7 +939,8 @@ TEST(Future, waitWithDuration) { Promise p; Future f = p.getFuture(); p.setValue(1); - auto t = f.wait(std::chrono::milliseconds(1)); + auto t = waitWithSemaphore(std::move(f), + std::chrono::milliseconds(1)); EXPECT_TRUE(t.isReady()); } { @@ -946,7 +948,8 @@ TEST(Future, waitWithDuration) { v_fb.push_back(makeFuture(true)); v_fb.push_back(makeFuture(false)); auto f = whenAll(v_fb.begin(), v_fb.end()); - auto t = f.wait(std::chrono::milliseconds(1)); + auto t = waitWithSemaphore(std::move(f), + std::chrono::milliseconds(1)); EXPECT_TRUE(t.isReady()); EXPECT_EQ(2, t.value().size()); } @@ -957,7 +960,8 @@ TEST(Future, waitWithDuration) { v_fb.push_back(p1.getFuture()); v_fb.push_back(p2.getFuture()); auto f = whenAll(v_fb.begin(), v_fb.end()); - auto t = f.wait(std::chrono::milliseconds(1)); + auto t = waitWithSemaphore(std::move(f), + std::chrono::milliseconds(1)); EXPECT_FALSE(t.isReady()); p1.setValue(true); EXPECT_FALSE(t.isReady()); @@ -965,7 +969,8 @@ TEST(Future, waitWithDuration) { EXPECT_TRUE(t.isReady()); } { - auto t = makeFuture().wait(std::chrono::milliseconds(1)); + auto t = waitWithSemaphore(makeFuture(), + std::chrono::milliseconds(1)); EXPECT_TRUE(t.isReady()); } } @@ -1220,7 +1225,7 @@ TEST(Future, t5506504) { return whenAll(futures.begin(), futures.end()); }; - fn().wait(); + waitWithSemaphore(fn()); } // Test of handling of a circular dependency. It's never recommended -- 2.34.1