From 5b4015ccbab12f17335b2ef91cb9bf65bc77f7fb Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 6 May 2015 13:19:00 -0700 Subject: [PATCH] Reference safety for Worker after callbacks Joining a WorkRequest now ensures that either the after callback has already been executed, or will never be executed. Fixes #1438 --- src/mbgl/util/work_request.cpp | 12 ++++-- src/mbgl/util/work_request.hpp | 4 +- src/mbgl/util/worker.cpp | 11 +++++- src/mbgl/util/worker.hpp | 8 ++-- test/miscellaneous/worker.cpp | 70 ++++++++++++++++++++++++++++++++++ test/test.gypi | 1 + 6 files changed, 96 insertions(+), 10 deletions(-) create mode 100644 test/miscellaneous/worker.cpp diff --git a/src/mbgl/util/work_request.cpp b/src/mbgl/util/work_request.cpp index 05fe53580..a7d4b9c02 100644 --- a/src/mbgl/util/work_request.cpp +++ b/src/mbgl/util/work_request.cpp @@ -4,22 +4,25 @@ namespace mbgl { WorkRequest::WorkRequest() = default; -WorkRequest::WorkRequest(Future&& future) - : complete(std::move(future)) { +WorkRequest::WorkRequest(Future&& future, JoinedFlag flag) + : complete(std::move(future)), + joinedFlag(flag) { } WorkRequest::WorkRequest(WorkRequest&& o) - : complete(std::move(o.complete)) { + : complete(std::move(o.complete)), + joinedFlag(std::move(o.joinedFlag)) { } WorkRequest::~WorkRequest() { if (complete.valid()) { - complete.get(); + join(); } } WorkRequest& WorkRequest::operator=(WorkRequest&& o) { complete = std::move(o.complete); + joinedFlag = std::move(o.joinedFlag); return *this; } @@ -28,6 +31,7 @@ WorkRequest::operator bool() const { } void WorkRequest::join() { + *joinedFlag = true; complete.get(); } diff --git a/src/mbgl/util/work_request.hpp b/src/mbgl/util/work_request.hpp index 8c26abb60..f0f461b5d 100644 --- a/src/mbgl/util/work_request.hpp +++ b/src/mbgl/util/work_request.hpp @@ -10,9 +10,10 @@ namespace mbgl { class WorkRequest : public util::noncopyable { public: using Future = std::future; + using JoinedFlag = std::shared_ptr>; WorkRequest(); - WorkRequest(Future&&); + WorkRequest(Future&&, JoinedFlag); WorkRequest(WorkRequest&&); ~WorkRequest(); @@ -24,6 +25,7 @@ public: private: Future complete; + JoinedFlag joinedFlag; }; } diff --git a/src/mbgl/util/worker.cpp b/src/mbgl/util/worker.cpp index 7f5620e1f..ad26ea5b1 100644 --- a/src/mbgl/util/worker.cpp +++ b/src/mbgl/util/worker.cpp @@ -27,10 +27,17 @@ WorkRequest Worker::send(Fn work, Fn after) { std::packaged_task task(work); std::future future = task.get_future(); - threads[current]->invokeWithResult(&Worker::Impl::doWork, std::move(after), task); + std::shared_ptr> joined = std::make_shared>(); + *joined = false; + + threads[current]->invokeWithResult(&Worker::Impl::doWork, [joined, after] { + if (!*joined) { + after(); + } + }, task); current = (current + 1) % threads.size(); - return WorkRequest(std::move(future)); + return WorkRequest(std::move(future), joined); } } diff --git a/src/mbgl/util/worker.hpp b/src/mbgl/util/worker.hpp index c4a4c376e..461ba36e5 100644 --- a/src/mbgl/util/worker.hpp +++ b/src/mbgl/util/worker.hpp @@ -21,9 +21,11 @@ public: // work is complete. // // The return value represents the request to perform the work asynchronously. - // Its destructor guarantees that the work is either cancelled and will never - // execute, or has finished executing. In other words, the WorkRequest is - // guaranteed to outlive any references held by the work function. + // Its destructor guarantees that the work function has finished executing, and + // that the after function has either finished executing or will not execute. + // Together, this means that an object may make a work request with lambdas which + // bind references to itself, and if and when those lambdas execute, the references + // will still be valid. WorkRequest send(Fn work, Fn after); private: diff --git a/test/miscellaneous/worker.cpp b/test/miscellaneous/worker.cpp new file mode 100644 index 000000000..50fabcebc --- /dev/null +++ b/test/miscellaneous/worker.cpp @@ -0,0 +1,70 @@ +#include "../fixtures/util.hpp" + +#include +#include + +using namespace mbgl; +using namespace mbgl::util; + +TEST(Worker, ExecutesWorkAndAfter) { + RunLoop loop(uv_default_loop()); + + Worker worker(1); + WorkRequest request; + + bool didWork = false; + bool didAfter = false; + + loop.invoke([&] { + request = worker.send([&] { + didWork = true; + }, [&] { + didAfter = true; + loop.stop(); + }); + }); + + uv_run(uv_default_loop(), UV_RUN_DEFAULT); + EXPECT_TRUE(didWork); + EXPECT_TRUE(didAfter); +} + +TEST(Worker, WorkRequestJoinWaitsForWorkToComplete) { + RunLoop loop(uv_default_loop()); + + Worker worker(1); + bool didWork = false; + + loop.invoke([&] { + WorkRequest request = worker.send([&] { + usleep(10000); + didWork = true; + }, [&] {}); + + request.join(); + EXPECT_TRUE(didWork); + loop.stop(); + }); + + uv_run(uv_default_loop(), UV_RUN_DEFAULT); +} + +TEST(Worker, WorkRequestJoinCancelsAfter) { + RunLoop loop(uv_default_loop()); + + Worker worker(1); + bool didAfter = false; + + loop.invoke([&] { + WorkRequest request = worker.send([&] { + }, [&] { + didAfter = true; + }); + + request.join(); + loop.stop(); + }); + + uv_run(uv_default_loop(), UV_RUN_DEFAULT); + EXPECT_FALSE(didAfter); +} diff --git a/test/test.gypi b/test/test.gypi index 3898090c7..9c6abb7d3 100644 --- a/test/test.gypi +++ b/test/test.gypi @@ -53,6 +53,7 @@ 'miscellaneous/text_conversions.cpp', 'miscellaneous/tile.cpp', 'miscellaneous/variant.cpp', + 'miscellaneous/worker.cpp', 'storage/storage.hpp', 'storage/storage.cpp',