Commit c4a221ce authored by James Sedgwick's avatar James Sedgwick Committed by woo

Another stab at waitWithSemaphore -> Future<T>::wait()

Summary:
See D1785572. Check out the new Future test and the commented portion of wait(Duration) for the fix

The test only fails a few times out of a hundred before the fix, but hasn't failed yet after

Test Plan: futures unit, wait for contbuild

Reviewed By: hans@fb.com

Subscribers: trunkagent, rushix, fbcode-common-diffs@, hero-diffs@, cold-storage-diffs@, adamsyta, zhuohuang, darshan, micha, folly-diffs@, lins, tingy, hannesr, jsedgwick

FB internal diff: D1803526

Tasks: 5940008, 6059995

Signature: t1:1803526:1422309486:3613c59a708ecac312d241723828763feb2a57aa
parent c182b25f
...@@ -510,65 +510,6 @@ whenN(InputIterator first, InputIterator last, size_t n) { ...@@ -510,65 +510,6 @@ whenN(InputIterator first, InputIterator last, size_t n) {
return ctx->p.getFuture(); return ctx->p.getFuture();
} }
template <typename T>
Future<T>
waitWithSemaphore(Future<T>&& f) {
Baton<> baton;
auto done = f.then([&](Try<T> &&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<void> waitWithSemaphore<void>(Future<void>&& f) {
Baton<> baton;
auto done = f.then([&](Try<void> &&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 <typename T, class Dur>
Future<T>
waitWithSemaphore(Future<T>&& f, Dur timeout) {
auto baton = std::make_shared<Baton<>>();
auto done = f.then([baton](Try<T> &&t) {
baton->post();
return std::move(t.value());
});
baton->timed_wait(std::chrono::system_clock::now() + timeout);
return done;
}
template <class Dur>
Future<void>
waitWithSemaphore(Future<void>&& f, Dur timeout) {
auto baton = std::make_shared<Baton<>>();
auto done = f.then([baton](Try<void> &&t) {
baton->post();
t.value();
});
baton->timed_wait(std::chrono::system_clock::now() + timeout);
return done;
}
namespace { namespace {
template <class T> template <class T>
void getWaitHelper(Future<T>* f) { void getWaitHelper(Future<T>* f) {
...@@ -741,7 +682,14 @@ Future<T> Future<T>::wait(Duration dur) { ...@@ -741,7 +682,14 @@ Future<T> Future<T>::wait(Duration dur) {
baton->post(); baton->post();
return makeFuture(std::move(t)); return makeFuture(std::move(t));
}); });
baton->timed_wait(std::chrono::system_clock::now() + dur); // Let's preserve the invariant that if we did not timeout (timed_wait returns
// true), then the returned Future is complete when it is returned to the
// caller. We need to wait out the race for that Future to complete.
if (baton->timed_wait(std::chrono::system_clock::now() + dur)) {
while (!done.isReady()) {
std::this_thread::yield();
}
}
return done; return done;
} }
......
...@@ -545,24 +545,6 @@ Future<std::vector<std::pair< ...@@ -545,24 +545,6 @@ Future<std::vector<std::pair<
Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>> Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>>
whenN(InputIterator first, InputIterator last, size_t n); 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 <class T>
Future<T> waitWithSemaphore(Future<T>&& 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 <typename T, class Dur>
Future<T> waitWithSemaphore(Future<T>&& f, Dur timeout);
} // folly } // folly
#include <folly/futures/Future-inl.h> #include <folly/futures/Future-inl.h>
...@@ -912,31 +912,31 @@ TEST(Future, throwIfFailed) { ...@@ -912,31 +912,31 @@ TEST(Future, throwIfFailed) {
}); });
} }
TEST(Future, waitWithSemaphoreImmediate) { TEST(Future, waitImmediate) {
waitWithSemaphore(makeFuture()); makeFuture().wait();
auto done = waitWithSemaphore(makeFuture(42)).value(); auto done = makeFuture(42).wait().value();
EXPECT_EQ(42, done); EXPECT_EQ(42, done);
vector<int> v{1,2,3}; vector<int> 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.size(), done_v.size());
EXPECT_EQ(v, done_v); EXPECT_EQ(v, done_v);
vector<Future<void>> v_f; vector<Future<void>> v_f;
v_f.push_back(makeFuture()); v_f.push_back(makeFuture());
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()); EXPECT_EQ(2, done_v_f.size());
vector<Future<bool>> v_fb; vector<Future<bool>> v_fb;
v_fb.push_back(makeFuture(true)); v_fb.push_back(makeFuture(true));
v_fb.push_back(makeFuture(false)); v_fb.push_back(makeFuture(false));
auto fut = whenAll(v_fb.begin(), v_fb.end()); 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()); EXPECT_EQ(2, done_v_fb.size());
} }
TEST(Future, waitWithSemaphore) { TEST(Future, wait) {
Promise<int> p; Promise<int> p;
Future<int> f = p.getFuture(); Future<int> f = p.getFuture();
std::atomic<bool> flag{false}; std::atomic<bool> flag{false};
...@@ -949,7 +949,7 @@ TEST(Future, waitWithSemaphore) { ...@@ -949,7 +949,7 @@ TEST(Future, waitWithSemaphore) {
return t.value(); return t.value();
}); });
flag = true; flag = true;
result.store(waitWithSemaphore(std::move(n)).value()); result.store(n.wait().value());
}, },
std::move(f) std::move(f)
); );
...@@ -963,12 +963,11 @@ TEST(Future, waitWithSemaphore) { ...@@ -963,12 +963,11 @@ TEST(Future, waitWithSemaphore) {
EXPECT_EQ(result.load(), 42); EXPECT_EQ(result.load(), 42);
} }
TEST(Future, waitWithSemaphoreForTime) { TEST(Future, waitWithDuration) {
{ {
Promise<int> p; Promise<int> p;
Future<int> f = p.getFuture(); Future<int> f = p.getFuture();
auto t = waitWithSemaphore(std::move(f), auto t = f.wait(std::chrono::milliseconds(1));
std::chrono::microseconds(1));
EXPECT_FALSE(t.isReady()); EXPECT_FALSE(t.isReady());
p.setValue(1); p.setValue(1);
EXPECT_TRUE(t.isReady()); EXPECT_TRUE(t.isReady());
...@@ -977,8 +976,7 @@ TEST(Future, waitWithSemaphoreForTime) { ...@@ -977,8 +976,7 @@ TEST(Future, waitWithSemaphoreForTime) {
Promise<int> p; Promise<int> p;
Future<int> f = p.getFuture(); Future<int> f = p.getFuture();
p.setValue(1); p.setValue(1);
auto t = waitWithSemaphore(std::move(f), auto t = f.wait(std::chrono::milliseconds(1));
std::chrono::milliseconds(1));
EXPECT_TRUE(t.isReady()); EXPECT_TRUE(t.isReady());
} }
{ {
...@@ -986,8 +984,7 @@ TEST(Future, waitWithSemaphoreForTime) { ...@@ -986,8 +984,7 @@ TEST(Future, waitWithSemaphoreForTime) {
v_fb.push_back(makeFuture(true)); v_fb.push_back(makeFuture(true));
v_fb.push_back(makeFuture(false)); v_fb.push_back(makeFuture(false));
auto f = whenAll(v_fb.begin(), v_fb.end()); auto f = whenAll(v_fb.begin(), v_fb.end());
auto t = waitWithSemaphore(std::move(f), auto t = f.wait(std::chrono::milliseconds(1));
std::chrono::milliseconds(1));
EXPECT_TRUE(t.isReady()); EXPECT_TRUE(t.isReady());
EXPECT_EQ(2, t.value().size()); EXPECT_EQ(2, t.value().size());
} }
...@@ -998,8 +995,7 @@ TEST(Future, waitWithSemaphoreForTime) { ...@@ -998,8 +995,7 @@ TEST(Future, waitWithSemaphoreForTime) {
v_fb.push_back(p1.getFuture()); v_fb.push_back(p1.getFuture());
v_fb.push_back(p2.getFuture()); v_fb.push_back(p2.getFuture());
auto f = whenAll(v_fb.begin(), v_fb.end()); auto f = whenAll(v_fb.begin(), v_fb.end());
auto t = waitWithSemaphore(std::move(f), auto t = f.wait(std::chrono::milliseconds(1));
std::chrono::milliseconds(1));
EXPECT_FALSE(t.isReady()); EXPECT_FALSE(t.isReady());
p1.setValue(true); p1.setValue(true);
EXPECT_FALSE(t.isReady()); EXPECT_FALSE(t.isReady());
...@@ -1007,10 +1003,36 @@ TEST(Future, waitWithSemaphoreForTime) { ...@@ -1007,10 +1003,36 @@ TEST(Future, waitWithSemaphoreForTime) {
EXPECT_TRUE(t.isReady()); EXPECT_TRUE(t.isReady());
} }
{ {
auto t = waitWithSemaphore(makeFuture(), auto t = makeFuture().wait(std::chrono::milliseconds(1));
std::chrono::milliseconds(1));
EXPECT_TRUE(t.isReady()); EXPECT_TRUE(t.isReady());
} }
{
Promise<void> p;
auto start = std::chrono::steady_clock::now();
auto f = p.getFuture().wait(std::chrono::milliseconds(100));
auto elapsed = std::chrono::steady_clock::now() - start;
EXPECT_GE(elapsed, std::chrono::milliseconds(100));
EXPECT_FALSE(f.isReady());
p.setValue();
EXPECT_TRUE(f.isReady());
}
{
// Try to trigger the race where the resultant Future is not yet complete
// even if we didn't hit the timeout, and make sure we deal with it properly
Promise<void> p;
folly::Baton<> b;
auto t = std::thread([&]{
b.post();
std::this_thread::sleep_for(std::chrono::milliseconds(100));
p.setValue();
});
b.wait();
auto f = p.getFuture().wait(std::chrono::seconds(3600));
EXPECT_TRUE(f.isReady());
t.join();
}
} }
class DummyDrivableExecutor : public DrivableExecutor { class DummyDrivableExecutor : public DrivableExecutor {
...@@ -1263,7 +1285,7 @@ TEST(Future, t5506504) { ...@@ -1263,7 +1285,7 @@ TEST(Future, t5506504) {
return whenAll(futures.begin(), futures.end()); return whenAll(futures.begin(), futures.end());
}; };
waitWithSemaphore(fn()); fn().wait();
} }
// Test of handling of a circular dependency. It's never recommended // Test of handling of a circular dependency. It's never recommended
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment