From aecef6da1a7903ef318e02f62d1cdd97a2e3e53b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= <mail@kkaefer.com>
Date: Fri, 13 Feb 2015 10:54:36 -0800
Subject: [PATCH] make sure that the async handle always gets closed

---
 include/mbgl/storage/default/request.hpp |  3 +-
 src/mbgl/storage/request.cpp             | 44 +++++++++++++++++-------
 test/storage/http_reading.cpp            | 28 +++++++++++++++
 3 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/include/mbgl/storage/default/request.hpp b/include/mbgl/storage/default/request.hpp
index 81ed14a56..e26d59e2d 100644
--- a/include/mbgl/storage/default/request.hpp
+++ b/include/mbgl/storage/default/request.hpp
@@ -22,7 +22,6 @@ class Request : private util::noncopyable {
 public:
     using Callback = std::function<void(const Response &)>;
     Request(const Resource &resource, uv_loop_t *loop, Callback callback);
-    ~Request();
 
 public:
     // May be called from any thread.
@@ -33,6 +32,8 @@ public:
     void cancel();
 
 private:
+    ~Request();
+    void invoke();
     static void notifyCallback(uv_async_t *async);
     static void cancelCallback(uv_async_t *async);
 
diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp
index 02c2822d7..09d28ca05 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);
diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp
index 3aa57cd80..ad87db15f 100644
--- a/test/storage/http_reading.cpp
+++ b/test/storage/http_reading.cpp
@@ -37,3 +37,31 @@ TEST_F(Storage, HTTPReading) {
 
     uv_run(uv_default_loop(), UV_RUN_DEFAULT);
 }
+
+TEST_F(Storage, HTTPNoCallback) {
+    SCOPED_TEST(HTTPTest)
+
+    using namespace mbgl;
+
+    DefaultFileSource fs(nullptr, uv_default_loop());
+
+    fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), nullptr);
+
+    uv_run(uv_default_loop(), UV_RUN_DEFAULT);
+
+    HTTPTest.finish();
+}
+
+TEST_F(Storage, HTTPNoCallbackNoLoop) {
+    SCOPED_TEST(HTTPTest)
+
+    using namespace mbgl;
+
+    DefaultFileSource fs(nullptr, uv_default_loop());
+
+    fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, nullptr);
+
+    uv_run(uv_default_loop(), UV_RUN_DEFAULT);
+
+    HTTPTest.finish();
+}