From: Hans Fugal Date: Fri, 5 Dec 2014 17:33:30 +0000 (-0800) Subject: via and activate/deactivate chaining X-Git-Tag: v0.22.0~130 X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=f2ae6aa71242720bd3e38dec9d9c827e9023c29a via and activate/deactivate chaining Summary: Better support and test chaining of `via` and `activate`/`deactivate`. The real problem is that without the ref qualifier a multiple chain of lvalue references can end up with a destructed object being referenced after. https://akrzemi1.wordpress.com/2014/06/02/ref-qualifiers/ Test Plan: This is mostly new tests that would fail and now pass. I think maybe the tests are a bit weak, it would be good to find a way to ensure we aren't going to see the access-after-free bugs in these tests. Reviewed By: jsedgwick@fb.com Subscribers: trunkagent, fugalh, exa, njormrod, folly-diffs@ FB internal diff: D1714873 Tasks: 5489801 Signature: t1:1714873:1417628538:9e610c5ba5e0a22c19a11d53aa956be45d585058 --- diff --git a/folly/wangle/Future-inl.h b/folly/wangle/Future-inl.h index 06d225b0..2df6fef2 100644 --- a/folly/wangle/Future-inl.h +++ b/folly/wangle/Future-inl.h @@ -328,7 +328,7 @@ Try& Future::getTry() { template template -inline Future Future::via(Executor* executor) { +inline Future Future::via(Executor* executor) && { throwIfInvalid(); this->deactivate(); @@ -337,6 +337,17 @@ inline Future Future::via(Executor* executor) { return std::move(*this); } +template +template +inline Future Future::via(Executor* executor) & { + throwIfInvalid(); + + MoveWrapper> p; + auto f = p->getFuture(); + then([p](Try&& t) mutable { p->fulfilTry(std::move(t)); }); + return std::move(f).via(executor); +} + template bool Future::isReady() const { throwIfInvalid(); diff --git a/folly/wangle/Future.h b/folly/wangle/Future.h index 57f8636b..f124d7b5 100644 --- a/folly/wangle/Future.h +++ b/folly/wangle/Future.h @@ -91,8 +91,17 @@ class Future { /// /// f = f.via(e).then(a); /// f.then(b); + // The ref-qualifier allows for `this` to be moved out so we + // don't get access-after-free situations in chaining. + // https://akrzemi1.wordpress.com/2014/06/02/ref-qualifiers/ template - Future via(Executor* executor); + Future via(Executor* executor) &&; + + /// This variant creates a new future, where the ref-qualifier && version + /// moves `this` out. This one is less efficient but avoids confusing users + /// when "return f.via(x);" fails. + template + Future via(Executor* executor) &; /** True when the result (or exception) is ready. */ bool isReady() const; @@ -322,6 +331,7 @@ class Future { core_->deactivate(); return std::move(*this); } + bool isActive() { return core_->isActive(); } diff --git a/folly/wangle/test/FutureTest.cpp b/folly/wangle/test/FutureTest.cpp index db15993c..eaae5da1 100644 --- a/folly/wangle/test/FutureTest.cpp +++ b/folly/wangle/test/FutureTest.cpp @@ -829,17 +829,14 @@ TEST(Future, callbackAfterActivate) { } TEST(Future, activateOnDestruct) { - Promise p; - auto f = p.getFuture(); - f.deactivate(); + auto f = std::make_shared>(makeFuture()); + f->deactivate(); size_t count = 0; - f.then([&](Try&&) { count++; }); - - p.setValue(); + f->then([&](Try&&) { count++; }); EXPECT_EQ(0, count); - f = makeFuture(); // force destruction of old f + f.reset(); EXPECT_EQ(1, count); } diff --git a/folly/wangle/test/ViaTest.cpp b/folly/wangle/test/ViaTest.cpp index 9a3758c2..3e9953e0 100644 --- a/folly/wangle/test/ViaTest.cpp +++ b/folly/wangle/test/ViaTest.cpp @@ -36,7 +36,6 @@ struct ManualWaiter { struct ViaFixture : public testing::Test { ViaFixture() : - future_(makeFuture().deactivate()), westExecutor(new ManualExecutor), eastExecutor(new ManualExecutor), waiter(new ManualWaiter(westExecutor)), @@ -61,7 +60,6 @@ struct ViaFixture : public testing::Test { }); } - Future future_; std::shared_ptr westExecutor; std::shared_ptr eastExecutor; std::shared_ptr waiter; @@ -117,9 +115,30 @@ TEST(Via, then_function) { EXPECT_EQ(f.value(), "start;static;class-static;class"); } +TEST_F(ViaFixture, deactivateChain) { + bool flag = false; + auto f = makeFuture().deactivate(); + EXPECT_FALSE(f.isActive()); + auto f2 = f.then([&](Try){ flag = true; }); + EXPECT_FALSE(flag); +} + +TEST_F(ViaFixture, deactivateActivateChain) { + bool flag = false; + // you can do this all day long with temporaries. + auto f1 = makeFuture().deactivate().activate().deactivate(); + // Chaining on activate/deactivate requires an rvalue, so you have to move + // one of these two ways (if you're not using a temporary). + auto f2 = std::move(f1).activate(); + f2.deactivate(); + auto f3 = std::move(f2.activate()); + f3.then([&](Try){ flag = true; }); + EXPECT_TRUE(flag); +} + TEST_F(ViaFixture, thread_hops) { auto westThreadId = std::this_thread::get_id(); - auto f = future_.via(eastExecutor.get()).then([=](Try&& t) { + auto f = via(eastExecutor.get()).then([=](Try&& t) { EXPECT_NE(std::this_thread::get_id(), westThreadId); return makeFuture(1); }).via(westExecutor.get() @@ -135,7 +154,7 @@ TEST_F(ViaFixture, thread_hops) { TEST_F(ViaFixture, chain_vias) { auto westThreadId = std::this_thread::get_id(); - auto f = future_.via(eastExecutor.get()).then([=](Try&& t) { + auto f = via(eastExecutor.get()).then([=](Try&& t) { EXPECT_NE(std::this_thread::get_id(), westThreadId); return makeFuture(1); }).then([=](Try&& t) { @@ -156,3 +175,12 @@ TEST_F(ViaFixture, chain_vias) { EXPECT_EQ(f.value(), 1); } +TEST_F(ViaFixture, bareViaAssignment) { + auto f = via(eastExecutor.get()); +} +TEST_F(ViaFixture, viaAssignment) { + // via()&& + auto f = makeFuture().via(eastExecutor.get()); + // via()& + auto f2 = f.via(eastExecutor.get()); +}