Merge pull request #1412 from mapbox/1309-another-try

Ensure worker tasks are complete before destroying TileData

.travis.yml

@@ -7,23 +7,23 @@ matrix:
     - os: linux
   include:
     - os: linux
-      env: FLAVOR=linux CXX=g++-4.8 BUILDTYPE=Release
+      env: FLAVOR=linux CXX=g++-4.9 BUILDTYPE=Release
       addons:
         apt:
           sources: [ 'ubuntu-toolchain-r-test' ]
-          packages: [ 'gdb', 'g++-4.8', 'gcc-4.8', 'libllvm3.4', 'xutils-dev', 'libxxf86vm-dev', 'x11proto-xf86vidmode-dev', 'mesa-utils' ]
+          packages: [ 'gdb', 'g++-4.9', 'gcc-4.9', 'libllvm3.4', 'xutils-dev', 'libxxf86vm-dev', 'x11proto-xf86vidmode-dev', 'mesa-utils' ]
     - os: linux
-      env: FLAVOR=linux CXX=clang++ BUILDTYPE=Debug
+      env: FLAVOR=linux CXX=clang++-3.5 BUILDTYPE=Debug
       addons:
         apt:
-          sources: [ 'ubuntu-toolchain-r-test' ]
-          packages: [ 'gdb', 'libgcc-4.8-dev', 'libstdc++-4.8-dev', 'libstdc++6', 'libllvm3.4', 'xutils-dev', 'libxxf86vm-dev', 'x11proto-xf86vidmode-dev', 'mesa-utils' ]
+          sources: [ 'ubuntu-toolchain-r-test', 'llvm-toolchain-precise-3.5' ]
+          packages: [ 'gdb', 'clang-3.5', 'libstdc++-4.9-dev', 'libstdc++6', 'libllvm3.4', 'xutils-dev', 'libxxf86vm-dev', 'x11proto-xf86vidmode-dev', 'mesa-utils' ]
     - os: linux
-      env: FLAVOR=linux CXX=clang++ BUILDTYPE=Release
+      env: FLAVOR=linux CXX=clang++-3.5 BUILDTYPE=Release
       addons:
         apt:
-          sources: [ 'ubuntu-toolchain-r-test' ]
-          packages: [ 'gdb', 'libgcc-4.8-dev', 'libstdc++-4.8-dev', 'libstdc++6', 'libllvm3.4', 'xutils-dev', 'libxxf86vm-dev', 'x11proto-xf86vidmode-dev', 'mesa-utils' ]
+          sources: [ 'ubuntu-toolchain-r-test', 'llvm-toolchain-precise-3.5' ]
+          packages: [ 'gdb', 'clang-3.5', 'libstdc++-4.9-dev', 'libstdc++6', 'libllvm3.4', 'xutils-dev', 'libxxf86vm-dev', 'x11proto-xf86vidmode-dev', 'mesa-utils' ]
     - os: osx
       env: FLAVOR=osx BUILDTYPE=Debug
     - os: osx

README.md

@@ -5,7 +5,7 @@ implemented in C++11, currently targeting iOS, OS X, Android, and Ubuntu Linux.
 
 # Depends
 
- - Modern C++ compiler that supports `-std=c++11` (On OS X clang++, on Linux g++-4.8 or g++-4.9)
+ - Modern C++ compiler that supports `-std=c++14` (On OS X clang++, on Linux g++-4.9)
  - [Boost headers](http://boost.org/)
  - [`zlib`](http://www.zlib.net/)
  - [`libpng`](http://www.libpng.org/pub/png/libpng.html)
@@ -85,8 +85,8 @@ Install GCC 4.8+ if you are running Ubuntu 13.10 or older. Alternatively, you ca
 
     sudo add-apt-repository --yes ppa:ubuntu-toolchain-r/test
     sudo apt-get update
-    sudo apt-get install gcc-4.8 g++-4.8
-    export CXX=g++-4.8
+    sudo apt-get install gcc-4.9 g++-4.9
+    export CXX=g++-4.9
 
 Ensure you have git and other build essentials:
 

gyp/common.gypi

@@ -8,7 +8,7 @@
       ['OS=="mac"', {
         'xcode_settings': {
           'CLANG_CXX_LIBRARY': 'libc++',
-          'CLANG_CXX_LANGUAGE_STANDARD': 'c++11',
+          'CLANG_CXX_LANGUAGE_STANDARD': 'c++14',
           'GCC_VERSION': 'com.apple.compilers.llvm.clang.1_0',
           'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',
           'GCC_ENABLE_CPP_RTTI': 'YES',
@@ -27,14 +27,14 @@
         },
       }, {
         'cflags_cc': [
-          '-std=c++11',
+          '-std=c++14',
           '-Werror',
           '-Wall',
           '-Wextra',
           '-Wshadow',
           '-Wno-variadic-macros',
           '-Wno-error=unused-parameter',
-          '-Wno-unknown-warning-option',
+          '-Wno-c++1y-extensions',
           '-frtti',
           '-fexceptions',
         ],

src/mbgl/map/live_tile_data.cpp

@@ -24,7 +24,11 @@ LiveTileData::LiveTileData(const TileID& id_,
     state = State::loaded;
 }
 
-LiveTileData::~LiveTileData() {}
+LiveTileData::~LiveTileData() {
+    // Cancel in most derived class destructor so that worker tasks are joined before
+    // any member data goes away.
+    cancel();
+}
 
 void LiveTileData::parse() {
     if (state != State::loaded) {

src/mbgl/map/raster_tile_data.cpp

@@ -9,6 +9,9 @@ RasterTileData::RasterTileData(const TileID& id_, TexturePool &texturePool,
 }
 
 RasterTileData::~RasterTileData() {
+    // Cancel in most derived class destructor so that worker tasks are joined before
+    // any member data goes away.
+    cancel();
 }
 
 void RasterTileData::parse() {

src/mbgl/map/tile_data.cpp

@@ -55,21 +55,11 @@ void TileData::cancel() {
         env.cancelRequest(req);
         req = nullptr;
     }
+    if (workRequest) {
+        workRequest.join();
+    }
 }
 
 void TileData::reparse(Worker& worker, std::function<void()> callback) {
-    util::ptr<TileData> tile = shared_from_this();
-    worker.send(
-        [this]() {
-            EnvironmentScope scope(env, ThreadType::TileWorker, "TileWorker_" + name);
-            parse();
-        },
-        [tile, callback]() {
-             // `tile` is bound in this lambda to ensure that if it's the last owning pointer,
-             // destruction happens on the map thread, not the worker thread. It is _not_ bound
-             // in the above lambda, because we do not want the possibility to arise that the
-             // after callback could execute and release the penultimate reference before the
-             // work callback has been destructed.
-            callback();
-        });
+    workRequest = worker.send([this] { parse(); }, callback);
 }

src/mbgl/map/tile_data.hpp

@@ -7,6 +7,7 @@
 
 #include <mbgl/util/noncopyable.hpp>
 #include <mbgl/util/ptr.hpp>
+#include <mbgl/util/work_request.hpp>
 
 #include <atomic>
 #include <string>
@@ -21,8 +22,7 @@ class StyleLayer;
 class Request;
 class Worker;
 
-class TileData : public std::enable_shared_from_this<TileData>,
-             private util::noncopyable {
+class TileData : private util::noncopyable {
 public:
     enum class State {
         invalid,
@@ -60,6 +60,8 @@ protected:
     Request *req = nullptr;
     std::string data;
 
+    WorkRequest workRequest;
+
     // Contains the tile ID string for painting debug information.
     DebugFontBuffer debugFontBuffer;
 

src/mbgl/map/vector_tile_data.cpp

@@ -28,6 +28,9 @@ VectorTileData::VectorTileData(const TileID& id_,
 }
 
 VectorTileData::~VectorTileData() {
+    // Cancel in most derived class destructor so that worker tasks are joined before
+    // any member data goes away.
+    cancel();
     glyphAtlas.removeGlyphs(reinterpret_cast<uintptr_t>(this));
 }
 

src/mbgl/util/run_loop.hpp

@@ -33,21 +33,10 @@ public:
         RunLoop* outer = current.get();
         assert(outer);
 
-        invoke([fn, callback, outer] {
-            /*
-                With C++14, we could write:
-
-                outer->invoke([cb = std::move(callback), result = std::move(fn())] () mutable {
-                    cb(std::move(result));
-                });
-
-                Instead we're using a workaround with std::bind
-                to obtain move-capturing semantics with C++11:
-                  http://stackoverflow.com/a/12744730/52207
-            */
-            outer->invoke(std::bind([] (std::function<void (R)>& cb, R& result) {
-                cb(std::move(result));
-            }, std::move(callback), std::move(fn())));
+        invoke([fn = std::move(fn), callback = std::move(callback), outer] () mutable {
+            outer->invoke([callback = std::move(callback), result = std::move(fn())] () mutable {
+                callback(std::move(result));
+            });
         });
     }
 
@@ -57,7 +46,7 @@ public:
         RunLoop* outer = current.get();
         assert(outer);
 
-        invoke([fn, callback, outer] {
+        invoke([fn = std::move(fn), callback = std::move(callback), outer] () mutable {
             fn();
             outer->invoke(std::move(callback));
         });

src/mbgl/util/thread.hpp

@@ -56,13 +56,13 @@ public:
     // Invoke object->fn(args...) in the runloop thread, then invoke callback(result) in the current thread.
     template <typename Fn, class R, class... Args>
     void invokeWithResult(Fn fn, std::function<void (R)>&& callback, Args&&... args) {
-        loop->invokeWithResult(std::bind(fn, object, args...), std::move(callback));
+        loop->invokeWithResult(std::bind(fn, object, std::move(args)...), std::move(callback));
     }
 
     // Invoke object->fn(args...) in the runloop thread, then invoke callback() in the current thread.
     template <typename Fn, class... Args>
     void invokeWithResult(Fn fn, std::function<void ()>&& callback, Args&&... args) {
-        loop->invokeWithResult(std::bind(fn, object, args...), std::move(callback));
+        loop->invokeWithResult(std::bind(fn, object, std::move(args)...), std::move(callback));
     }
 
     // Invoke object->fn(args...) in the runloop thread, and wait for the result.

src/mbgl/util/uv_detail.hpp

@@ -75,8 +75,8 @@ public:
     }
 
     inline ~handle() {
-        uv_close(t.release(), [](uv_handle_t* handle) {
-            delete reinterpret_cast<T*>(handle);
+        uv_close(t.release(), [](uv_handle_t* h) {
+            delete reinterpret_cast<T*>(h);
         });
     }
 

src/mbgl/util/work_request.cpp

@@ -0,0 +1,34 @@
+#include <mbgl/util/work_request.hpp>
+
+namespace mbgl {
+
+WorkRequest::WorkRequest() = default;
+
+WorkRequest::WorkRequest(Future&& future)
+    : complete(std::move(future)) {
+}
+
+WorkRequest::WorkRequest(WorkRequest&& o)
+    : complete(std::move(o.complete)) {
+}
+
+WorkRequest::~WorkRequest() {
+    if (complete.valid()) {
+        complete.get();
+    }
+}
+
+WorkRequest& WorkRequest::operator=(WorkRequest&& o) {
+    complete = std::move(o.complete);
+    return *this;
+}
+
+WorkRequest::operator bool() const {
+    return complete.valid();
+}
+
+void WorkRequest::join() {
+    complete.get();
+}
+
+}

src/mbgl/util/work_request.hpp

@@ -0,0 +1,31 @@
+#ifndef MBGL_UTIL_WORK_REQUEST
+#define MBGL_UTIL_WORK_REQUEST
+
+#include <mbgl/util/noncopyable.hpp>
+
+#include <future>
+
+namespace mbgl {
+
+class WorkRequest : public util::noncopyable {
+public:
+    using Future = std::future<void>;
+
+    WorkRequest();
+    WorkRequest(Future&&);
+    WorkRequest(WorkRequest&&);
+    ~WorkRequest();
+
+    WorkRequest& operator=(WorkRequest&&);
+    operator bool() const;
+
+    // Wait for the worker task to complete.
+    void join();
+
+private:
+    Future complete;
+};
+
+}
+
+#endif

src/mbgl/util/worker.cpp

@@ -2,6 +2,7 @@
 #include <mbgl/platform/platform.hpp>
 
 #include <cassert>
+#include <future>
 
 namespace mbgl {
 
@@ -9,8 +10,8 @@ class Worker::Impl {
 public:
     Impl(uv_loop_t*) {}
 
-    void doWork(Fn work) {
-        work();
+    void doWork(std::packaged_task<void ()>& task) {
+        task();
     }
 };
 
@@ -22,9 +23,14 @@ Worker::Worker(std::size_t count) {
 
 Worker::~Worker() = default;
 
-void Worker::send(Fn&& work, Fn&& after) {
-    threads[current]->invokeWithResult(&Worker::Impl::doWork, std::move(after), std::move(work));
+WorkRequest Worker::send(Fn work, Fn after) {
+    std::packaged_task<void ()> task(work);
+    std::future<void> future = task.get_future();
+
+    threads[current]->invokeWithResult(&Worker::Impl::doWork, std::move(after), task);
+
     current = (current + 1) % threads.size();
+    return WorkRequest(std::move(future));
 }
 
 }

src/mbgl/util/worker.hpp

@@ -2,6 +2,7 @@
 #define MBGL_UTIL_WORKER
 
 #include <mbgl/util/noncopyable.hpp>
+#include <mbgl/util/work_request.hpp>
 #include <mbgl/util/thread.hpp>
 
 #include <functional>
@@ -15,7 +16,15 @@ public:
     Worker(std::size_t count);
     ~Worker();
 
-    void send(Fn&& work, Fn&& after);
+    // Request work be done on a thread pool. The optional after callback is
+    // executed on the invoking thread, which must have a run loop, after the
+    // 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.
+    WorkRequest send(Fn work, Fn after);
 
 private:
     class Impl;