diff --git a/include/mbgl/storage/default/request.hpp b/include/mbgl/storage/default/request.hpp index 81ed14a56..648585f30 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; Request(const Resource &resource, uv_loop_t *loop, Callback callback); - ~Request(); public: // May be called from any thread. @@ -33,12 +32,14 @@ public: void cancel(); private: + ~Request(); + void invoke(); static void notifyCallback(uv_async_t *async); static void cancelCallback(uv_async_t *async); private: - uv_async_t *notify_async = nullptr; - uv_async_t *destruct_async = nullptr; + uv_async_t *notifyAsync = nullptr; + uv_async_t *destructAsync = nullptr; Callback callback; std::shared_ptr response; diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index c1a57e725..de18138ec 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -18,12 +18,12 @@ Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_) // 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) { - notify_async = new uv_async_t; - notify_async->data = this; + notifyAsync = new uv_async_t; + notifyAsync->data = nullptr; #if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10 - uv_async_init(loop, notify_async, [](uv_async_t *async, int) { notifyCallback(async); }); + uv_async_init(loop, notifyAsync, [](uv_async_t *async, int) { notifyCallback(async); }); #else - uv_async_init(loop, notify_async, notifyCallback); + uv_async_init(loop, notifyAsync, notifyCallback); #endif } } @@ -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(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; + 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(); } 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,54 +48,72 @@ 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 &response_) { response = response_; - if (notify_async) { - uv_async_send(notify_async); + assert(response); + if (notifyAsync) { + assert(!notifyAsync->data); + notifyAsync->data = this; + uv_async_send(notifyAsync); } 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(); } } void Request::cancel() { MBGL_VERIFY_THREAD(tid) - assert(notify_async); - assert(!destruct_async); - destruct_async = new uv_async_t; - destruct_async->data = this; + assert(notifyAsync); + assert(!destructAsync); + destructAsync = new uv_async_t; + destructAsync->data = nullptr; #if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10 - uv_async_init(notify_async->loop, destruct_async, [](uv_async_t *async, int) { cancelCallback(async); }); + uv_async_init(notifyAsync->loop, destructAsync, [](uv_async_t *async, int) { cancelCallback(async); }); #else - uv_async_init(notify_async->loop, destruct_async, cancelCallback); + uv_async_init(notifyAsync->loop, destructAsync, cancelCallback); #endif } void Request::cancelCallback(uv_async_t *async) { - // The destruct_async will be invoked *after* the notify_async callback has already run. + // The destructAsync will be invoked *after* the notifyAsync callback has already run. auto request = reinterpret_cast(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() { - if (notify_async) { - notify(nullptr); + 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); } - assert(destruct_async); - uv_async_send(destruct_async); + // This will finally destruct this object. + assert(!destructAsync->data); + destructAsync->data = this; + uv_async_send(destructAsync); } } 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(); +}