make sure that the async handle always gets closed

1 files changed, 32 insertions, 12 deletions

diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp
index 02c2822d76..09d28ca055 100644
--- a/src/mbgl/storage/request.cpp
+++ b/src/mbgl/storage/request.cpp
@@ -31,13 +31,15 @@ Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_)
 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->destruct_async) {
-        // We haven't created a cancel request, so we can safely delete this Request object
-        // since it won't be accessed in the future.
-        assert(request->response);
-        request->callback(*request->response);
-        delete request;
+        // 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();
     } 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
@@ -46,24 +48,30 @@ void Request::notifyCallback(uv_async_t *async) {
     }
 }
 
+void Request::invoke() {
+    assert(response);
+    // The user could supply a null pointer or empty std::function as a callback. In this case, we
+    // still do the file request, but we don't need to deliver a result.
+    if (callback) {
+        callback(*response);
+    }
+    delete this;
+}
 
 Request::~Request() {
-    if (notify_async) {
-        // Request objects can be destructed in other threads when the user didn't supply a loop.
-        MBGL_VERIFY_THREAD(tid)
-    }
 }
 
 void Request::notify(const std::shared_ptr<const Response> &response_) {
     response = response_;
+    assert(response);
     if (notify_async) {
         assert(!notify_async->data);
         notify_async->data = this;
         uv_async_send(notify_async);
     } else {
-        assert(response);
-        callback(*response);
-        delete this;
+        // This request is not cancelable. This means that the callback will be executed in an
+        // arbitrary thread (== FileSource thread).
+        invoke();
     }
 }
 
@@ -84,13 +92,25 @@ void Request::cancelCallback(uv_async_t *async) {
     // The destruct_async will be invoked *after* the notify_async 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.
 void Request::destruct() {
+    assert(notify_async);
     assert(destruct_async);
+
+    if (!notify_async->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 destruct_async handle as well.
+        notify_async->data = this;
+        uv_async_send(notify_async);
+    }
+
+    // This will finally destruct this object.
     assert(!destruct_async->data);
     destruct_async->data = this;
     uv_async_send(destruct_async);