fix rare issue that had the notify and cancel callbacks called in the wrong order

1 files changed, 44 insertions, 57 deletions

diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp
index 48b5b774d2..ed7f625e86 100644
--- a/src/mbgl/storage/request.cpp
+++ b/src/mbgl/storage/request.cpp
@@ -3,6 +3,7 @@
 #include <mbgl/storage/response.hpp>
 
 #include <mbgl/util/util.hpp>
+#include <mbgl/util/std.hpp>
 #include <mbgl/util/uv.hpp>
 
 #include <uv.h>
@@ -12,39 +13,39 @@
 
 namespace mbgl {
 
+struct Request::Canceled { std::mutex mutex; bool confirmed = false; };
+
 // Note: This requires that loop is running in the current thread (or not yet running).
 Request::Request(const Resource &resource_, uv_loop_t *loop, const Environment &env_, Callback callback_)
     : callback(callback_), resource(resource_), env(env_) {
     // When there is no loop supplied (== nullptr), the callback will be fired in an arbitrary
     // thread (the thread notify() is called from) rather than kicking back to the calling thread.
     if (loop) {
-        notifyAsync = new uv_async_t;
-        notifyAsync->data = nullptr;
+        async = new uv_async_t;
+        async->data = this;
 #if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10
-        uv_async_init(loop, notifyAsync, [](uv_async_t *async, int) { notifyCallback(async); });
+        uv_async_init(loop, async, [](uv_async_t *a, int) { reinterpret_cast<Request *>(a->data)->notifyCallback(); });
 #else
-        uv_async_init(loop, notifyAsync, notifyCallback);
+        uv_async_init(loop, async, [](uv_async_t *a) { reinterpret_cast<Request *>(a->data)->notifyCallback(); });
 #endif
     }
 }
 
-void Request::notifyCallback(uv_async_t *async) {
-    auto request = reinterpret_cast<Request *>(async->data);
-    uv::close(async);
-    assert(request);
-    MBGL_VERIFY_THREAD(request->tid)
-
-    if (!request->destructAsync) {
-        // Call the callback with the result data. This will also delete this object. We haven't
-        // created a cancel request, so this is safe since it won't be accessed in the future.
-        // It is up to the user to not call cancel() on this Request object after the response was
-        // delivered.
-        request->invoke();
+// Called in the originating thread.
+void Request::notifyCallback() {
+    MBGL_VERIFY_THREAD(tid)
+    if (!canceled) {
+        invoke();
     } else {
-        // Otherwise, we're waiting for for the destruct notification to be delivered in order
-        // to delete the Request object. We're doing this since we can't know whether the
-        // DefaultFileSource is still sending a cancel event, which means this object must still
-        // exist.
+        bool destroy = false;
+        {
+            std::unique_lock<std::mutex> lock(canceled->mutex);
+            destroy = canceled->confirmed;
+        }
+        // Don't delete right way, because we have to unlock the mutex before deleting.
+        if (destroy) {
+            delete this;
+        }
     }
 }
 
@@ -59,15 +60,21 @@ void Request::invoke() {
 }
 
 Request::~Request() {
+    if (async) {
+        uv_close(reinterpret_cast<uv_handle_t*>(async), [](uv_handle_t* handle) {
+            delete reinterpret_cast<uv_async_t*>(handle);
+        });
+    }
 }
 
+// Called in the FileSource thread.
 void Request::notify(const std::shared_ptr<const Response> &response_) {
+    assert(!response);
     response = response_;
     assert(response);
-    if (notifyAsync) {
-        assert(!notifyAsync->data);
-        notifyAsync->data = this;
-        uv_async_send(notifyAsync);
+
+    if (async) {
+        uv_async_send(async);
     } else {
         // This request is not cancelable. This means that the callback will be executed in an
         // arbitrary thread (== FileSource thread).
@@ -75,45 +82,25 @@ void Request::notify(const std::shared_ptr<const Response> &response_) {
     }
 }
 
+// Called in the originating thread.
 void Request::cancel() {
     MBGL_VERIFY_THREAD(tid)
-    assert(notifyAsync);
-    assert(!destructAsync);
-    destructAsync = new uv_async_t;
-    destructAsync->data = nullptr;
-#if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10
-    uv_async_init(notifyAsync->loop, destructAsync, [](uv_async_t *async, int) { cancelCallback(async); });
-#else
-    uv_async_init(notifyAsync->loop, destructAsync, cancelCallback);
-#endif
+    assert(async);
+    assert(!canceled);
+    canceled = util::make_unique<Canceled>();
 }
 
-void Request::cancelCallback(uv_async_t *async) {
-    // The destructAsync will be invoked *after* the notifyAsync callback has already run.
-    auto request = reinterpret_cast<Request *>(async->data);
-    uv::close(async);
-    assert(request);
-    MBGL_VERIFY_THREAD(request->tid)
-    delete request;
-}
 
-// This gets called from the FileSource thread, and will only ever be invoked after cancel() was called
-// in the original requesting thread.
+// Called in the FileSource thread.
+// Will only ever be invoked after cancel() was called in the original requesting thread.
 void Request::destruct() {
-    assert(notifyAsync);
-    assert(destructAsync);
-
-    if (!notifyAsync->data) {
-        // The async hasn't been triggered yet, but we need to so that it'll close the handle. The
-        // callback will not delete this object since we have a destructAsync handle as well.
-        notifyAsync->data = this;
-        uv_async_send(notifyAsync);
-    }
-
-    // This will finally destruct this object.
-    assert(!destructAsync->data);
-    destructAsync->data = this;
-    uv_async_send(destructAsync);
+    assert(async);
+    assert(canceled);
+    std::unique_lock<std::mutex> lock(canceled->mutex);
+    canceled->confirmed = true;
+    uv_async_send(async);
+    // after this method returns, the FileSource thread has no knowledge of
+    // this object anymore.
 }
 
 }