From 9a0005fdea5c7eb9107793305f475d233b0669c2 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Thu, 16 Apr 2015 12:24:14 -0700 Subject: [PATCH 1/9] Remove unnecessary moves / casts --- platform/default/sqlite_cache.cpp | 4 ++-- src/mbgl/storage/default_file_source.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index a57e666e9..4450374a3 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -183,9 +183,9 @@ void SQLiteCache::put(const Resource &resource, std::shared_ptr // storing a new response or updating the currently stored response, potentially setting a new // expiry date. if (hint == Hint::Full) { - thread->invoke(&Impl::put, resource, std::move(response)); + thread->invoke(&Impl::put, resource, response); } else if (hint == Hint::Refresh) { - thread->invoke(&Impl::refresh, resource, int64_t(response->expires)); + thread->invoke(&Impl::refresh, resource, response->expires); } } diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 4cbc98fa5..208a529bc 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -60,7 +60,7 @@ Request* DefaultFileSource::request(const Resource& resource, // This function can be called from any thread. Make sure we're executing the actual call in the // file source loop by sending it over the queue. - thread->invoke(&Impl::add, std::move(req), thread->get()); + thread->invoke(&Impl::add, req, thread->get()); return req; } @@ -74,7 +74,7 @@ void DefaultFileSource::cancel(Request *req) { // This function can be called from any thread. Make sure we're executing the actual call in the // file source loop by sending it over the queue. - thread->invoke(&Impl::cancel, std::move(req)); + thread->invoke(&Impl::cancel, req); } void DefaultFileSource::abort(const Environment &env) { From f4adef18891dff33a8d4e7ba9e14cc02c17fb6cf Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 13 Apr 2015 14:37:57 -0700 Subject: [PATCH 2/9] =?UTF-8?q?pointer=20=E2=87=A2=20reference?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- platform/darwin/http_request_nsurl.mm | 2 +- platform/default/asset_request_fs.cpp | 4 ++-- platform/default/asset_request_zip.cpp | 5 ++--- platform/default/http_request_curl.cpp | 2 +- src/mbgl/storage/asset_request.hpp | 2 +- src/mbgl/storage/default_file_source.cpp | 4 ++-- src/mbgl/storage/http_request.hpp | 2 +- src/mbgl/storage/shared_request_base.hpp | 14 +++++--------- 8 files changed, 15 insertions(+), 20 deletions(-) diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm index 0fd4a4e8f..24b31ae0a 100644 --- a/platform/darwin/http_request_nsurl.mm +++ b/platform/darwin/http_request_nsurl.mm @@ -397,7 +397,7 @@ void HTTPRequestImpl::restart(uv_timer_t *timer, int) { // ------------------------------------------------------------------------------------------------- -HTTPRequest::HTTPRequest(DefaultFileSource::Impl *source, const Resource &resource) +HTTPRequest::HTTPRequest(DefaultFileSource::Impl &source, const Resource &resource) : SharedRequestBase(source, resource) { } diff --git a/platform/default/asset_request_fs.cpp b/platform/default/asset_request_fs.cpp index 08b336ad3..fbb739c5c 100644 --- a/platform/default/asset_request_fs.cpp +++ b/platform/default/asset_request_fs.cpp @@ -62,7 +62,7 @@ AssetRequestImpl::AssetRequestImpl(AssetRequest *request_, uv_loop_t *loop) : re path = url.substr(8); } else { // This is a relative path. Prefix with the application root. - path = request->source->assetRoot + "/" + url.substr(8); + path = request->source.assetRoot + "/" + url.substr(8); } uv_fs_open(loop, &req, path.c_str(), O_RDONLY, S_IRUSR, fileOpened); @@ -207,7 +207,7 @@ void AssetRequestImpl::cleanup(uv_fs_t *req) { // ------------------------------------------------------------------------------------------------- -AssetRequest::AssetRequest(DefaultFileSource::Impl *source_, const Resource &resource_) +AssetRequest::AssetRequest(DefaultFileSource::Impl &source_, const Resource &resource_) : SharedRequestBase(source_, resource_) { assert(algo::starts_with(resource.url, "asset://")); } diff --git a/platform/default/asset_request_zip.cpp b/platform/default/asset_request_zip.cpp index f66ca619b..ba91809f1 100644 --- a/platform/default/asset_request_zip.cpp +++ b/platform/default/asset_request_zip.cpp @@ -114,7 +114,7 @@ AssetRequestImpl::~AssetRequestImpl() { AssetRequestImpl::AssetRequestImpl(AssetRequest *request_, uv_loop_t *loop) : context(*AssetZipContext::Get(loop)), request(request_), - root(request->source->assetRoot), + root(request->source.assetRoot), path(std::string { "assets/" } + request->resource.url.substr(8)) { auto zip = context.getHandle(root); if (zip) { @@ -129,7 +129,6 @@ void AssetRequestImpl::openZipArchive() { req->data = this; assert(request); - assert(request->source); // We're using uv_fs_open first to obtain a file descriptor. Then, uv_zip_fdopen will operate // on a read-only file. @@ -275,7 +274,7 @@ void AssetRequestImpl::cancel() { // ------------------------------------------------------------------------------------------------- -AssetRequest::AssetRequest(DefaultFileSource::Impl *source_, const Resource &resource_) +AssetRequest::AssetRequest(DefaultFileSource::Impl &source_, const Resource &resource_) : SharedRequestBase(source_, resource_) { assert(algo::starts_with(resource.url, "asset://")); } diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp index 1672c9cdb..a5de61104 100644 --- a/platform/default/http_request_curl.cpp +++ b/platform/default/http_request_curl.cpp @@ -722,7 +722,7 @@ void HTTPRequestImpl::handleResult(CURLcode code) { // ------------------------------------------------------------------------------------------------- -HTTPRequest::HTTPRequest(DefaultFileSource::Impl *source_, const Resource &resource_) +HTTPRequest::HTTPRequest(DefaultFileSource::Impl &source_, const Resource &resource_) : SharedRequestBase(source_, resource_) { } diff --git a/src/mbgl/storage/asset_request.hpp b/src/mbgl/storage/asset_request.hpp index 48d421c3b..22e63c1ad 100644 --- a/src/mbgl/storage/asset_request.hpp +++ b/src/mbgl/storage/asset_request.hpp @@ -7,7 +7,7 @@ namespace mbgl { class AssetRequest : public SharedRequestBase { public: - AssetRequest(DefaultFileSource::Impl *source, const Resource &resource); + AssetRequest(DefaultFileSource::Impl &source, const Resource &resource); void start(uv_loop_t *loop, std::shared_ptr response = nullptr); void cancel(); diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 208a529bc..cc48e8473 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -89,9 +89,9 @@ void DefaultFileSource::Impl::add(Request* req, uv_loop_t* loop) { if (!sharedRequest) { // There is no request for this URL yet. Create a new one and start it. if (algo::starts_with(resource.url, "asset://")) { - sharedRequest = new AssetRequest(this, resource); + sharedRequest = new AssetRequest(*this, resource); } else { - sharedRequest = new HTTPRequest(this, resource); + sharedRequest = new HTTPRequest(*this, resource); } const bool inserted = pending.emplace(resource, sharedRequest).second; diff --git a/src/mbgl/storage/http_request.hpp b/src/mbgl/storage/http_request.hpp index 54e9a77ef..6232bd22e 100644 --- a/src/mbgl/storage/http_request.hpp +++ b/src/mbgl/storage/http_request.hpp @@ -7,7 +7,7 @@ namespace mbgl { class HTTPRequest : public SharedRequestBase { public: - HTTPRequest(DefaultFileSource::Impl *source, const Resource &resource); + HTTPRequest(DefaultFileSource::Impl &source, const Resource &resource); void start(uv_loop_t *loop, std::shared_ptr response = nullptr); void cancel(); diff --git a/src/mbgl/storage/shared_request_base.hpp b/src/mbgl/storage/shared_request_base.hpp index d7ed00264..8fc23b7bd 100644 --- a/src/mbgl/storage/shared_request_base.hpp +++ b/src/mbgl/storage/shared_request_base.hpp @@ -26,7 +26,7 @@ protected: MBGL_STORE_THREAD(tid) public: - SharedRequestBase(DefaultFileSource::Impl *source_, const Resource &resource_) + SharedRequestBase(DefaultFileSource::Impl &source_, const Resource &resource_) : resource(resource_), source(source_) {} virtual void start(uv_loop_t *loop, std::shared_ptr response = nullptr) = 0; @@ -35,9 +35,7 @@ public: void notify(std::shared_ptr response, FileCache::Hint hint) { MBGL_VERIFY_THREAD(tid); - if (source) { - source->notify(this, observers, response, hint); - } + source.notify(this, observers, response, hint); } void subscribe(Request *request) { @@ -53,10 +51,8 @@ public: if (abandoned()) { // There are no observers anymore. We are initiating cancelation. - if (source) { - // First, remove this SharedRequestBase from the source. - source->notify(this, observers, nullptr, FileCache::Hint::No); - } + // First, remove this SharedRequestBase from the source. + source.notify(this, observers, nullptr, FileCache::Hint::No); // Then, initiate cancelation of this request cancel(); @@ -95,7 +91,7 @@ public: const Resource resource; protected: - DefaultFileSource::Impl *source = nullptr; + DefaultFileSource::Impl &source; private: std::set observers; From 2a8ddc9f27faded12c3013283123427af607f02e Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Thu, 16 Apr 2015 17:11:49 -0700 Subject: [PATCH 3/9] Keep observers encapsulated --- src/mbgl/storage/default_file_source.cpp | 14 +++----------- src/mbgl/storage/default_file_source_impl.hpp | 2 +- src/mbgl/storage/shared_request_base.hpp | 11 +++++++++-- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index cc48e8473..68a072f40 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -184,22 +184,14 @@ void DefaultFileSource::Impl::abort(const Environment& env) { } void DefaultFileSource::Impl::notify(SharedRequestBase *sharedRequest, - const std::set &observers, std::shared_ptr response, FileCache::Hint hint) { // First, remove the request, since it might be destructed at any point now. assert(find(sharedRequest->resource) == sharedRequest); pending.erase(sharedRequest->resource); - if (response) { - if (cache) { - // Store response in database - cache->put(sharedRequest->resource, response, hint); - } - - // Notify all observers. - for (auto req : observers) { - req->notify(response); - } + if (response && cache) { + // Store response in database + cache->put(sharedRequest->resource, response, hint); } } diff --git a/src/mbgl/storage/default_file_source_impl.hpp b/src/mbgl/storage/default_file_source_impl.hpp index 97210dc44..7a673b6ce 100644 --- a/src/mbgl/storage/default_file_source_impl.hpp +++ b/src/mbgl/storage/default_file_source_impl.hpp @@ -14,7 +14,7 @@ class DefaultFileSource::Impl { public: Impl(FileCache *cache, const std::string &root = ""); - void notify(SharedRequestBase *sharedRequest, const std::set &observers, + void notify(SharedRequestBase *sharedRequest, std::shared_ptr response, FileCache::Hint hint); SharedRequestBase *find(const Resource &resource); diff --git a/src/mbgl/storage/shared_request_base.hpp b/src/mbgl/storage/shared_request_base.hpp index 8fc23b7bd..3cd33b2ed 100644 --- a/src/mbgl/storage/shared_request_base.hpp +++ b/src/mbgl/storage/shared_request_base.hpp @@ -35,7 +35,14 @@ public: void notify(std::shared_ptr response, FileCache::Hint hint) { MBGL_VERIFY_THREAD(tid); - source.notify(this, observers, response, hint); + source.notify(this, response, hint); + + if (response) { + // Notify all observers. + for (auto req : observers) { + req->notify(response); + } + } } void subscribe(Request *request) { @@ -52,7 +59,7 @@ public: if (abandoned()) { // There are no observers anymore. We are initiating cancelation. // First, remove this SharedRequestBase from the source. - source.notify(this, observers, nullptr, FileCache::Hint::No); + source.notify(this, nullptr, FileCache::Hint::No); // Then, initiate cancelation of this request cancel(); From ea73c99faa7f46c2c958644fd1300d1cf88701b0 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Thu, 16 Apr 2015 17:55:44 -0700 Subject: [PATCH 4/9] Assert that notify gets a response --- src/mbgl/storage/shared_request_base.hpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/mbgl/storage/shared_request_base.hpp b/src/mbgl/storage/shared_request_base.hpp index 3cd33b2ed..3fc0c5bfe 100644 --- a/src/mbgl/storage/shared_request_base.hpp +++ b/src/mbgl/storage/shared_request_base.hpp @@ -34,14 +34,13 @@ public: void notify(std::shared_ptr response, FileCache::Hint hint) { MBGL_VERIFY_THREAD(tid); + assert(response); source.notify(this, response, hint); - if (response) { - // Notify all observers. - for (auto req : observers) { - req->notify(response); - } + // Notify all observers. + for (auto req : observers) { + req->notify(response); } } From b61cc6433b50c9a8a5083c1c5051e5c3ce78b761 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Thu, 16 Apr 2015 17:57:41 -0700 Subject: [PATCH 5/9] Make subscribe and unsubscribe parallel --- src/mbgl/storage/default_file_source.cpp | 11 ++++++++++- src/mbgl/storage/shared_request_base.hpp | 11 ----------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 68a072f40..6ead7781b 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -119,6 +119,15 @@ void DefaultFileSource::Impl::cancel(Request* req) { // unsubscribe callback triggers the removal of the SharedRequestBase pointer from the list // of pending requests and initiates cancelation. sharedRequest->unsubscribe(req); + + if (sharedRequest->abandoned()) { + // There are no observers anymore. We are initiating cancelation. + // First, remove this SharedRequestBase from the source. + pending.erase(sharedRequest->resource); + + // Then, initiate cancelation of this request + sharedRequest->cancel(); + } } else { // There is no request for this URL anymore. Likely, the request already completed // before we got around to process the cancelation request. @@ -189,7 +198,7 @@ void DefaultFileSource::Impl::notify(SharedRequestBase *sharedRequest, assert(find(sharedRequest->resource) == sharedRequest); pending.erase(sharedRequest->resource); - if (response && cache) { + if (cache) { // Store response in database cache->put(sharedRequest->resource, response, hint); } diff --git a/src/mbgl/storage/shared_request_base.hpp b/src/mbgl/storage/shared_request_base.hpp index 3fc0c5bfe..ce741ce11 100644 --- a/src/mbgl/storage/shared_request_base.hpp +++ b/src/mbgl/storage/shared_request_base.hpp @@ -46,23 +46,12 @@ public: void subscribe(Request *request) { MBGL_VERIFY_THREAD(tid); - observers.insert(request); } void unsubscribe(Request *request) { MBGL_VERIFY_THREAD(tid); - observers.erase(request); - - if (abandoned()) { - // There are no observers anymore. We are initiating cancelation. - // First, remove this SharedRequestBase from the source. - source.notify(this, nullptr, FileCache::Hint::No); - - // Then, initiate cancelation of this request - cancel(); - } } bool abandoned() const { From 359aca6624f183099a9afe58aae5a41bd2cb3772 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 17 Apr 2015 09:31:00 -0700 Subject: [PATCH 6/9] Privatize SharedRequestBase::source --- platform/default/asset_request_fs.cpp | 7 ++++--- platform/default/asset_request_zip.cpp | 7 ++++--- src/mbgl/storage/asset_request.hpp | 3 ++- src/mbgl/storage/default_file_source.cpp | 2 +- src/mbgl/storage/shared_request_base.hpp | 4 +--- 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/platform/default/asset_request_fs.cpp b/platform/default/asset_request_fs.cpp index fbb739c5c..446f2f3d5 100644 --- a/platform/default/asset_request_fs.cpp +++ b/platform/default/asset_request_fs.cpp @@ -62,7 +62,7 @@ AssetRequestImpl::AssetRequestImpl(AssetRequest *request_, uv_loop_t *loop) : re path = url.substr(8); } else { // This is a relative path. Prefix with the application root. - path = request->source.assetRoot + "/" + url.substr(8); + path = request->assetRoot + "/" + url.substr(8); } uv_fs_open(loop, &req, path.c_str(), O_RDONLY, S_IRUSR, fileOpened); @@ -207,8 +207,9 @@ void AssetRequestImpl::cleanup(uv_fs_t *req) { // ------------------------------------------------------------------------------------------------- -AssetRequest::AssetRequest(DefaultFileSource::Impl &source_, const Resource &resource_) - : SharedRequestBase(source_, resource_) { +AssetRequest::AssetRequest(DefaultFileSource::Impl &source_, const Resource &resource_, const std::string& assetRoot_) + : SharedRequestBase(source_, resource_) + , assetRoot(assetRoot_) { assert(algo::starts_with(resource.url, "asset://")); } diff --git a/platform/default/asset_request_zip.cpp b/platform/default/asset_request_zip.cpp index ba91809f1..8a471a4fc 100644 --- a/platform/default/asset_request_zip.cpp +++ b/platform/default/asset_request_zip.cpp @@ -114,7 +114,7 @@ AssetRequestImpl::~AssetRequestImpl() { AssetRequestImpl::AssetRequestImpl(AssetRequest *request_, uv_loop_t *loop) : context(*AssetZipContext::Get(loop)), request(request_), - root(request->source.assetRoot), + root(request->assetRoot), path(std::string { "assets/" } + request->resource.url.substr(8)) { auto zip = context.getHandle(root); if (zip) { @@ -274,8 +274,9 @@ void AssetRequestImpl::cancel() { // ------------------------------------------------------------------------------------------------- -AssetRequest::AssetRequest(DefaultFileSource::Impl &source_, const Resource &resource_) - : SharedRequestBase(source_, resource_) { +AssetRequest::AssetRequest(DefaultFileSource::Impl &source_, const Resource &resource_, const std::string& assetRoot_) + : SharedRequestBase(source_, resource_) + , assetRoot(assetRoot_) { assert(algo::starts_with(resource.url, "asset://")); } diff --git a/src/mbgl/storage/asset_request.hpp b/src/mbgl/storage/asset_request.hpp index 22e63c1ad..2a571d209 100644 --- a/src/mbgl/storage/asset_request.hpp +++ b/src/mbgl/storage/asset_request.hpp @@ -7,7 +7,7 @@ namespace mbgl { class AssetRequest : public SharedRequestBase { public: - AssetRequest(DefaultFileSource::Impl &source, const Resource &resource); + AssetRequest(DefaultFileSource::Impl&, const Resource&, const std::string& assetRoot); void start(uv_loop_t *loop, std::shared_ptr response = nullptr); void cancel(); @@ -15,6 +15,7 @@ public: private: ~AssetRequest(); void *ptr = nullptr; + const std::string& assetRoot; friend class AssetRequestImpl; }; diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 6ead7781b..f4c220e36 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -89,7 +89,7 @@ void DefaultFileSource::Impl::add(Request* req, uv_loop_t* loop) { if (!sharedRequest) { // There is no request for this URL yet. Create a new one and start it. if (algo::starts_with(resource.url, "asset://")) { - sharedRequest = new AssetRequest(*this, resource); + sharedRequest = new AssetRequest(*this, resource, assetRoot); } else { sharedRequest = new HTTPRequest(*this, resource); } diff --git a/src/mbgl/storage/shared_request_base.hpp b/src/mbgl/storage/shared_request_base.hpp index ce741ce11..cf70ac0f0 100644 --- a/src/mbgl/storage/shared_request_base.hpp +++ b/src/mbgl/storage/shared_request_base.hpp @@ -85,10 +85,8 @@ protected: public: const Resource resource; -protected: - DefaultFileSource::Impl &source; - private: + DefaultFileSource::Impl& source; std::set observers; }; From cdd8f3ac3235e1d5aab47a6d32c48a0b82a3a1b9 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 17 Apr 2015 09:58:24 -0700 Subject: [PATCH 7/9] Separate RequestBase from DefaultFileSource internals SharedRequestBase was trying to do two different jobs: be a base class for Asset/HttpRequest and track internal state for DefaultFileSource. Now we have RequestBase for the former, and DefaultFileRequest for the latter. --- platform/darwin/http_request_nsurl.mm | 13 +- platform/default/asset_request_fs.cpp | 11 +- platform/default/asset_request_zip.cpp | 13 +- platform/default/http_request_curl.cpp | 13 +- src/mbgl/storage/asset_request.hpp | 6 +- src/mbgl/storage/default_file_source.cpp | 214 +++++++++--------- src/mbgl/storage/default_file_source_impl.hpp | 37 +-- src/mbgl/storage/http_request.hpp | 8 +- src/mbgl/storage/request_base.hpp | 39 ++++ src/mbgl/storage/shared_request_base.hpp | 95 -------- 10 files changed, 194 insertions(+), 255 deletions(-) create mode 100644 src/mbgl/storage/request_base.hpp delete mode 100644 src/mbgl/storage/shared_request_base.hpp diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm index 24b31ae0a..07ac16e05 100644 --- a/platform/darwin/http_request_nsurl.mm +++ b/platform/darwin/http_request_nsurl.mm @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -397,36 +398,28 @@ void HTTPRequestImpl::restart(uv_timer_t *timer, int) { // ------------------------------------------------------------------------------------------------- -HTTPRequest::HTTPRequest(DefaultFileSource::Impl &source, const Resource &resource) - : SharedRequestBase(source, resource) { +HTTPRequest::HTTPRequest(const Resource& resource, Callback callback) + : RequestBase(resource, callback) { } HTTPRequest::~HTTPRequest() { - MBGL_VERIFY_THREAD(tid); - if (ptr) { reinterpret_cast(ptr)->cancel(); } } void HTTPRequest::start(uv_loop_t *loop, std::shared_ptr response) { - MBGL_VERIFY_THREAD(tid); - assert(!ptr); ptr = new HTTPRequestImpl(this, loop, response); } void HTTPRequest::retryImmediately() { - MBGL_VERIFY_THREAD(tid); - if (ptr) { reinterpret_cast(ptr)->retryImmediately(); } } void HTTPRequest::cancel() { - MBGL_VERIFY_THREAD(tid); - delete this; } diff --git a/platform/default/asset_request_fs.cpp b/platform/default/asset_request_fs.cpp index 446f2f3d5..9580c2f5b 100644 --- a/platform/default/asset_request_fs.cpp +++ b/platform/default/asset_request_fs.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -207,23 +208,19 @@ void AssetRequestImpl::cleanup(uv_fs_t *req) { // ------------------------------------------------------------------------------------------------- -AssetRequest::AssetRequest(DefaultFileSource::Impl &source_, const Resource &resource_, const std::string& assetRoot_) - : SharedRequestBase(source_, resource_) +AssetRequest::AssetRequest(const Resource& resource_, Callback callback_, const std::string& assetRoot_) + : RequestBase(resource_, callback_) , assetRoot(assetRoot_) { assert(algo::starts_with(resource.url, "asset://")); } AssetRequest::~AssetRequest() { - MBGL_VERIFY_THREAD(tid); - if (ptr) { reinterpret_cast(ptr)->request = nullptr; } } void AssetRequest::start(uv_loop_t *loop, std::shared_ptr response) { - MBGL_VERIFY_THREAD(tid); - // We're ignoring the existing response if any. (void(response)); @@ -233,8 +230,6 @@ void AssetRequest::start(uv_loop_t *loop, std::shared_ptr respon } void AssetRequest::cancel() { - MBGL_VERIFY_THREAD(tid); - if (ptr) { reinterpret_cast(ptr)->canceled = true; diff --git a/platform/default/asset_request_zip.cpp b/platform/default/asset_request_zip.cpp index 8a471a4fc..69de1c742 100644 --- a/platform/default/asset_request_zip.cpp +++ b/platform/default/asset_request_zip.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -107,7 +108,7 @@ AssetRequestImpl::~AssetRequestImpl() { MBGL_VERIFY_THREAD(tid); if (request) { - request->ptr = nullptr; + request->impl = nullptr; } } @@ -274,23 +275,19 @@ void AssetRequestImpl::cancel() { // ------------------------------------------------------------------------------------------------- -AssetRequest::AssetRequest(DefaultFileSource::Impl &source_, const Resource &resource_, const std::string& assetRoot_) - : SharedRequestBase(source_, resource_) +AssetRequest::AssetRequest(const Resource& resource_, Callback callback_, const std::string& assetRoot_) + : RequestBase(resource_, callback_) , assetRoot(assetRoot_) { assert(algo::starts_with(resource.url, "asset://")); } AssetRequest::~AssetRequest() { - MBGL_VERIFY_THREAD(tid); - if (ptr) { reinterpret_cast(ptr)->cancel(); } } void AssetRequest::start(uv_loop_t *loop, std::shared_ptr response) { - MBGL_VERIFY_THREAD(tid); - // We're ignoring the existing response if any. (void(response)); @@ -300,8 +297,6 @@ void AssetRequest::start(uv_loop_t *loop, std::shared_ptr respon } void AssetRequest::cancel() { - MBGL_VERIFY_THREAD(tid); - if (ptr) { reinterpret_cast(ptr)->cancel(); } diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp index a5de61104..ffe3b525a 100644 --- a/platform/default/http_request_curl.cpp +++ b/platform/default/http_request_curl.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -722,36 +723,28 @@ void HTTPRequestImpl::handleResult(CURLcode code) { // ------------------------------------------------------------------------------------------------- -HTTPRequest::HTTPRequest(DefaultFileSource::Impl &source_, const Resource &resource_) - : SharedRequestBase(source_, resource_) { +HTTPRequest::HTTPRequest(const Resource& resource_, Callback callback_) + : RequestBase(resource_, callback_) { } HTTPRequest::~HTTPRequest() { - MBGL_VERIFY_THREAD(tid); - if (ptr) { reinterpret_cast(ptr)->abandon(); } } void HTTPRequest::start(uv_loop_t *loop, std::shared_ptr response) { - MBGL_VERIFY_THREAD(tid); - assert(!ptr); ptr = new HTTPRequestImpl(this, loop, response); } void HTTPRequest::retryImmediately() { - MBGL_VERIFY_THREAD(tid); - if (ptr) { reinterpret_cast(ptr)->retryImmediately(); } } void HTTPRequest::cancel() { - MBGL_VERIFY_THREAD(tid); - if (ptr) { delete reinterpret_cast(ptr); ptr = nullptr; diff --git a/src/mbgl/storage/asset_request.hpp b/src/mbgl/storage/asset_request.hpp index 2a571d209..e20e1d818 100644 --- a/src/mbgl/storage/asset_request.hpp +++ b/src/mbgl/storage/asset_request.hpp @@ -1,13 +1,13 @@ #ifndef MBGL_STORAGE_DEFAULT_ASSET_REQUEST #define MBGL_STORAGE_DEFAULT_ASSET_REQUEST -#include "shared_request_base.hpp" +#include "request_base.hpp" namespace mbgl { -class AssetRequest : public SharedRequestBase { +class AssetRequest : public RequestBase { public: - AssetRequest(DefaultFileSource::Impl&, const Resource&, const std::string& assetRoot); + AssetRequest(const Resource&, Callback, const std::string& assetRoot); void start(uv_loop_t *loop, std::shared_ptr response = nullptr); void cancel(); diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index f4c220e36..c510b5c7f 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -28,10 +28,6 @@ namespace algo = boost::algorithm; namespace mbgl { -DefaultFileSource::Impl::Impl(FileCache* cache_, const std::string& root) - : assetRoot(root.empty() ? platform::assetRoot() : root), cache(cache_) { -} - DefaultFileSource::DefaultFileSource(FileCache* cache, const std::string& root) : thread(util::make_unique>("FileSource", cache, root)) { } @@ -40,18 +36,6 @@ DefaultFileSource::~DefaultFileSource() { MBGL_VERIFY_THREAD(tid); } -SharedRequestBase *DefaultFileSource::Impl::find(const Resource &resource) { - // We're using a set of pointers here instead of a map between url and SharedRequestBase because - // we need to find the requests both by pointer and by URL. Given that the number of requests - // is generally very small (typically < 10 at a time), hashing by URL incurs too much overhead - // anyway. - const auto it = pending.find(resource); - if (it != pending.end()) { - return it->second; - } - return nullptr; -} - Request* DefaultFileSource::request(const Resource& resource, uv_loop_t* l, const Environment& env, @@ -81,52 +65,95 @@ void DefaultFileSource::abort(const Environment &env) { thread->invoke(&Impl::abort, std::ref(env)); } -void DefaultFileSource::Impl::add(Request* req, uv_loop_t* loop) { - const Resource &resource = req->resource; +// ----- Impl ----- - // We're adding a new Request. - SharedRequestBase *sharedRequest = find(resource); - if (!sharedRequest) { - // There is no request for this URL yet. Create a new one and start it. - if (algo::starts_with(resource.url, "asset://")) { - sharedRequest = new AssetRequest(*this, resource, assetRoot); - } else { - sharedRequest = new HTTPRequest(*this, resource); - } +DefaultFileSource::Impl::Impl(FileCache* cache_, const std::string& root) + : cache(cache_), assetRoot(root.empty() ? platform::assetRoot() : root) { +} - const bool inserted = pending.emplace(resource, sharedRequest).second; - assert(inserted); - (void (inserted)); // silence unused variable warning on Release builds. - - // But first, we're going to start querying the database if it exists. - if (!cache) { - sharedRequest->start(loop); - } else { - // Otherwise, first check the cache for existing data so that we can potentially - // revalidate the information without having to redownload everything. - cache->get(resource, [this, resource, loop](std::unique_ptr response) { - processResult(resource, std::move(response), loop); - }); - } +DefaultFileRequest* DefaultFileSource::Impl::find(const Resource& resource) { + const auto it = pending.find(resource); + if (it != pending.end()) { + return &it->second; } - sharedRequest->subscribe(req); + return nullptr; +} + +void DefaultFileSource::Impl::add(Request* req, uv_loop_t* loop) { + const Resource& resource = req->resource; + DefaultFileRequest* request = find(resource); + + if (request) { + request->observers.insert(req); + return; + } + + request = &pending.emplace(resource, DefaultFileRequest(resource, loop)).first->second; + request->observers.insert(req); + + if (cache) { + startCacheRequest(resource); + } else { + startRealRequest(resource); + } +} + +void DefaultFileSource::Impl::startCacheRequest(const Resource& resource) { + // Check the cache for existing data so that we can potentially + // revalidate the information without having to redownload everything. + cache->get(resource, [this, resource](std::unique_ptr response) { + DefaultFileRequest* request = find(resource); + + if (!request) { + // There is no request for this URL anymore. Likely, the request was canceled + // before we got around to process the cache result. + return; + } + + auto expired = [&response] { + const int64_t now = std::chrono::duration_cast( + SystemClock::now().time_since_epoch()).count(); + return response->expires <= now; + }; + + if (!response || expired()) { + // No response or stale cache. Run the real request. + startRealRequest(resource, std::move(response)); + } else { + // The response is fresh. We're good to notify the caller. + notify(request, std::move(response), FileCache::Hint::No); + } + }); +} + +void DefaultFileSource::Impl::startRealRequest(const Resource& resource, std::shared_ptr response) { + DefaultFileRequest* request = find(resource); + + auto callback = [request, this] (std::shared_ptr res, FileCache::Hint hint) { + notify(request, res, hint); + }; + + if (algo::starts_with(resource.url, "asset://")) { + request->request = new AssetRequest(resource, callback, assetRoot); + } else { + request->request = new HTTPRequest(resource, callback); + } + + request->request->start(request->loop, response); } void DefaultFileSource::Impl::cancel(Request* req) { - SharedRequestBase *sharedRequest = find(req->resource); - if (sharedRequest) { - // If the number of dependent requests of the SharedRequestBase drops to zero, the - // unsubscribe callback triggers the removal of the SharedRequestBase pointer from the list - // of pending requests and initiates cancelation. - sharedRequest->unsubscribe(req); + DefaultFileRequest* request = find(req->resource); - if (sharedRequest->abandoned()) { - // There are no observers anymore. We are initiating cancelation. - // First, remove this SharedRequestBase from the source. - pending.erase(sharedRequest->resource); - - // Then, initiate cancelation of this request - sharedRequest->cancel(); + if (request) { + // If the number of dependent requests of the DefaultFileRequest drops to zero, + // cancel the request and remove it from the pending list. + request->observers.erase(req); + if (request->observers.empty()) { + if (request->request) { + request->request->cancel(); + } + pending.erase(request->resource); } } else { // There is no request for this URL anymore. Likely, the request already completed @@ -138,70 +165,51 @@ void DefaultFileSource::Impl::cancel(Request* req) { req->destruct(); } -void DefaultFileSource::Impl::processResult(const Resource& resource, std::shared_ptr response, uv_loop_t* loop) { - SharedRequestBase *sharedRequest = find(resource); - if (sharedRequest) { - if (response) { - // This entry was stored in the cache. Now determine if we need to revalidate. - const int64_t now = std::chrono::duration_cast( - SystemClock::now().time_since_epoch()).count(); - if (response->expires > now) { - // The response is fresh. We're good to notify the caller. - sharedRequest->notify(response, FileCache::Hint::No); - sharedRequest->cancel(); - return; - } else { - // The cached response is stale. Now run the real request. - sharedRequest->start(loop, response); - } - } else { - // There is no response. Now run the real request. - sharedRequest->start(loop); - } - } else { - // There is no request for this URL anymore. Likely, the request was canceled - // before we got around to process the cache result. - } -} - // Aborts all requests that are part of the current environment. void DefaultFileSource::Impl::abort(const Environment& env) { // Construct a cancellation response. - auto res = util::make_unique(); - res->status = Response::Error; - res->message = "Environment is terminating"; - std::shared_ptr response = std::move(res); + auto response = std::make_shared(); + response->status = Response::Error; + response->message = "Environment is terminating"; // Iterate through all pending requests and remove them in case they're abandoned. - util::erase_if(pending, [&](const std::pair &it) -> bool { - // Obtain all pending requests that are in the current environment. - const auto aborted = it.second->removeAllInEnvironment(env); + util::erase_if(pending, [&](std::pair& it) -> bool { + // Notify all pending requests that are in the current environment. + util::erase_if(it.second.observers, [&](Request* req) -> bool { + if (&req->env == &env) { + req->notify(response); + return true; + } else { + return false; + } + }); - // Notify all observers. - for (auto req : aborted) { - req->notify(response); + bool abandoned = it.second.observers.empty(); + + if (abandoned && it.second.request) { + it.second.request->cancel(); } - // Finally, remove all requests that are now abandoned. - if (it.second->abandoned()) { - it.second->cancel(); - return true; - } else { - return false; - } + return abandoned; }); } -void DefaultFileSource::Impl::notify(SharedRequestBase *sharedRequest, - std::shared_ptr response, FileCache::Hint hint) { +void DefaultFileSource::Impl::notify(DefaultFileRequest* request, std::shared_ptr response, FileCache::Hint hint) { // First, remove the request, since it might be destructed at any point now. - assert(find(sharedRequest->resource) == sharedRequest); - pending.erase(sharedRequest->resource); + assert(find(request->resource) == request); + assert(response); + + // Notify all observers. + for (auto req : request->observers) { + req->notify(response); + } if (cache) { // Store response in database - cache->put(sharedRequest->resource, response, hint); + cache->put(request->resource, response, hint); } + + pending.erase(request->resource); } } diff --git a/src/mbgl/storage/default_file_source_impl.hpp b/src/mbgl/storage/default_file_source_impl.hpp index 7a673b6ce..d3f83af38 100644 --- a/src/mbgl/storage/default_file_source_impl.hpp +++ b/src/mbgl/storage/default_file_source_impl.hpp @@ -8,27 +8,36 @@ namespace mbgl { -class SharedRequestBase; +class RequestBase; + +struct DefaultFileRequest { + const Resource resource; + uv_loop_t* loop; + std::set observers; + RequestBase* request = nullptr; + + DefaultFileRequest(const Resource& resource_, uv_loop_t* loop_) + : resource(resource_), loop(loop_) {} +}; class DefaultFileSource::Impl { public: - Impl(FileCache *cache, const std::string &root = ""); + Impl(FileCache*, const std::string& = ""); - void notify(SharedRequestBase *sharedRequest, - std::shared_ptr response, FileCache::Hint hint); - SharedRequestBase *find(const Resource &resource); - - void add(Request* request, uv_loop_t* loop); - void cancel(Request* request); - void abort(const Environment& env); - - const std::string assetRoot; + void add(Request*, uv_loop_t*); + void cancel(Request*); + void abort(const Environment&); private: - void processResult(const Resource& resource, std::shared_ptr response, uv_loop_t* loop); + DefaultFileRequest* find(const Resource&); - std::unordered_map pending; - FileCache *cache = nullptr; + void startCacheRequest(const Resource&); + void startRealRequest(const Resource&, std::shared_ptr = nullptr); + void notify(DefaultFileRequest*, std::shared_ptr, FileCache::Hint); + + std::unordered_map pending; + FileCache* cache = nullptr; + const std::string assetRoot; }; } diff --git a/src/mbgl/storage/http_request.hpp b/src/mbgl/storage/http_request.hpp index 6232bd22e..faf6ee2f1 100644 --- a/src/mbgl/storage/http_request.hpp +++ b/src/mbgl/storage/http_request.hpp @@ -1,13 +1,15 @@ #ifndef MBGL_STORAGE_DEFAULT_HTTP_REQUEST #define MBGL_STORAGE_DEFAULT_HTTP_REQUEST -#include "shared_request_base.hpp" +#include "request_base.hpp" namespace mbgl { -class HTTPRequest : public SharedRequestBase { +struct Resource; + +class HTTPRequest : public RequestBase { public: - HTTPRequest(DefaultFileSource::Impl &source, const Resource &resource); + HTTPRequest(const Resource&, Callback); void start(uv_loop_t *loop, std::shared_ptr response = nullptr); void cancel(); diff --git a/src/mbgl/storage/request_base.hpp b/src/mbgl/storage/request_base.hpp new file mode 100644 index 000000000..281e76a02 --- /dev/null +++ b/src/mbgl/storage/request_base.hpp @@ -0,0 +1,39 @@ +#ifndef MBGL_STORAGE_REQUEST +#define MBGL_STORAGE_REQUEST + +#include +#include + +#include +#include + +typedef struct uv_loop_s uv_loop_t; + +namespace mbgl { + +struct Resource; +class Response; + +class RequestBase : private util::noncopyable { +public: + using Callback = std::function response, FileCache::Hint hint)>; + + RequestBase(const Resource& resource_, Callback notify_) + : notify(notify_) + , resource(resource_) { + } + + virtual ~RequestBase() = default; + + virtual void start(uv_loop_t *loop, std::shared_ptr response = nullptr) = 0; + virtual void cancel() = 0; + + Callback notify; + +protected: + const Resource& resource; +}; + +} + +#endif diff --git a/src/mbgl/storage/shared_request_base.hpp b/src/mbgl/storage/shared_request_base.hpp deleted file mode 100644 index cf70ac0f0..000000000 --- a/src/mbgl/storage/shared_request_base.hpp +++ /dev/null @@ -1,95 +0,0 @@ -#ifndef MBGL_STORAGE_DEFAULT_SHARED_REQUEST_BASE -#define MBGL_STORAGE_DEFAULT_SHARED_REQUEST_BASE - -#include -#include -#include -#include -#include -#include - -#include -#include -#include -#include - -typedef struct uv_loop_s uv_loop_t; - -namespace mbgl { - -class Request; -class Response; -class DefaultFileSource; - -class SharedRequestBase : private util::noncopyable { -protected: - MBGL_STORE_THREAD(tid) - -public: - SharedRequestBase(DefaultFileSource::Impl &source_, const Resource &resource_) - : resource(resource_), source(source_) {} - - virtual void start(uv_loop_t *loop, std::shared_ptr response = nullptr) = 0; - virtual void cancel() = 0; - - void notify(std::shared_ptr response, FileCache::Hint hint) { - MBGL_VERIFY_THREAD(tid); - assert(response); - - source.notify(this, response, hint); - - // Notify all observers. - for (auto req : observers) { - req->notify(response); - } - } - - void subscribe(Request *request) { - MBGL_VERIFY_THREAD(tid); - observers.insert(request); - } - - void unsubscribe(Request *request) { - MBGL_VERIFY_THREAD(tid); - observers.erase(request); - } - - bool abandoned() const { - return observers.empty(); - } - - std::vector removeAllInEnvironment(const Environment &env) { - MBGL_VERIFY_THREAD(tid); - - std::vector result; - - // Removes all Requests in the supplied environment and returns a list - // of them. - util::erase_if(observers, [&](Request *req) -> bool { - if (&req->env == &env) { - result.push_back(req); - return true; - } else { - return false; - } - }); - - return result; - } - -protected: - virtual ~SharedRequestBase() { - MBGL_VERIFY_THREAD(tid); - } - -public: - const Resource resource; - -private: - DefaultFileSource::Impl& source; - std::set observers; -}; - -} - -#endif From 8e5fa341ebdc1699f08a1dbf4b0dfd98a9969948 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 17 Apr 2015 12:37:43 -0700 Subject: [PATCH 8/9] Remove RequestBase::start Requests are always started immediately upon construction. While here, use typed pointers, not void*. --- platform/darwin/http_request_nsurl.mm | 23 +++++----- platform/default/asset_request_fs.cpp | 54 +++++++++--------------- platform/default/asset_request_zip.cpp | 28 +++++------- platform/default/http_request_curl.cpp | 28 +++++------- src/mbgl/storage/asset_request.hpp | 12 +++--- src/mbgl/storage/default_file_source.cpp | 6 +-- src/mbgl/storage/http_request.hpp | 12 +++--- src/mbgl/storage/request_base.hpp | 11 ++--- 8 files changed, 70 insertions(+), 104 deletions(-) diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm index 07ac16e05..f5e7888f5 100644 --- a/platform/darwin/http_request_nsurl.mm +++ b/platform/darwin/http_request_nsurl.mm @@ -200,7 +200,7 @@ void HTTPRequestImpl::handleResponse() { } context->removeRequest(request); - request->ptr = nullptr; + request->impl = nullptr; delete request; request = nullptr; } @@ -243,7 +243,7 @@ HTTPRequestImpl::~HTTPRequestImpl() { if (request) { context->removeRequest(request); - request->ptr = nullptr; + request->impl = nullptr; } } @@ -398,24 +398,21 @@ void HTTPRequestImpl::restart(uv_timer_t *timer, int) { // ------------------------------------------------------------------------------------------------- -HTTPRequest::HTTPRequest(const Resource& resource, Callback callback) - : RequestBase(resource, callback) { +HTTPRequest::HTTPRequest(const Resource& resource, Callback callback, + uv_loop_t* loop, std::shared_ptr response) + : RequestBase(resource, callback) + , impl(new HTTPRequestImpl(this, loop, response)) { } HTTPRequest::~HTTPRequest() { - if (ptr) { - reinterpret_cast(ptr)->cancel(); + if (impl) { + impl->cancel(); } } -void HTTPRequest::start(uv_loop_t *loop, std::shared_ptr response) { - assert(!ptr); - ptr = new HTTPRequestImpl(this, loop, response); -} - void HTTPRequest::retryImmediately() { - if (ptr) { - reinterpret_cast(ptr)->retryImmediately(); + if (impl) { + impl->retryImmediately(); } } diff --git a/platform/default/asset_request_fs.cpp b/platform/default/asset_request_fs.cpp index 9580c2f5b..87e432062 100644 --- a/platform/default/asset_request_fs.cpp +++ b/platform/default/asset_request_fs.cpp @@ -26,7 +26,7 @@ class AssetRequestImpl { MBGL_STORE_THREAD(tid) public: - AssetRequestImpl(AssetRequest *request, uv_loop_t *loop); + AssetRequestImpl(AssetRequest*, uv_loop_t*, const std::string& assetRoot); ~AssetRequestImpl(); static void fileOpened(uv_fs_t *req); @@ -36,8 +36,8 @@ public: static void notifyError(uv_fs_t *req); static void cleanup(uv_fs_t *req); - AssetRequest *request = nullptr; + std::string assetRoot; bool canceled = false; uv_fs_t req; uv_file fd = -1; @@ -49,11 +49,13 @@ AssetRequestImpl::~AssetRequestImpl() { MBGL_VERIFY_THREAD(tid); if (request) { - request->ptr = nullptr; + request->impl = nullptr; } } -AssetRequestImpl::AssetRequestImpl(AssetRequest *request_, uv_loop_t *loop) : request(request_) { +AssetRequestImpl::AssetRequestImpl(AssetRequest* request_, uv_loop_t* loop, const std::string& assetRoot_) + : request(request_) + , assetRoot(assetRoot_) { req.data = this; const auto &url = request->resource.url; @@ -63,7 +65,7 @@ AssetRequestImpl::AssetRequestImpl(AssetRequest *request_, uv_loop_t *loop) : re path = url.substr(8); } else { // This is a relative path. Prefix with the application root. - path = request->assetRoot + "/" + url.substr(8); + path = assetRoot + "/" + url.substr(8); } uv_fs_open(loop, &req, path.c_str(), O_RDONLY, S_IRUSR, fileOpened); @@ -208,40 +210,26 @@ void AssetRequestImpl::cleanup(uv_fs_t *req) { // ------------------------------------------------------------------------------------------------- -AssetRequest::AssetRequest(const Resource& resource_, Callback callback_, const std::string& assetRoot_) +AssetRequest::AssetRequest(const Resource& resource_, Callback callback_, + uv_loop_t* loop, const std::string& assetRoot) : RequestBase(resource_, callback_) - , assetRoot(assetRoot_) { + , impl(new AssetRequestImpl(this, loop, assetRoot)) { assert(algo::starts_with(resource.url, "asset://")); -} - -AssetRequest::~AssetRequest() { - if (ptr) { - reinterpret_cast(ptr)->request = nullptr; - } -} - -void AssetRequest::start(uv_loop_t *loop, std::shared_ptr response) { - // We're ignoring the existing response if any. - (void(response)); - - assert(!ptr); - ptr = new AssetRequestImpl(this, loop); // Note: the AssetRequestImpl deletes itself. } -void AssetRequest::cancel() { - if (ptr) { - reinterpret_cast(ptr)->canceled = true; - - // uv_cancel fails frequently when the request has already been started. - // In that case, we have to let it complete and check the canceled bool - // instead. The cancelation callback will delete the AssetRequest object. - uv_cancel((uv_req_t *)&reinterpret_cast(ptr)->req); - } else { - // This request is canceled before we called start. We're safe to delete - // ourselves now. - delete this; +AssetRequest::~AssetRequest() { + if (impl) { + impl->request = nullptr; } } +void AssetRequest::cancel() { + impl->canceled = true; + // uv_cancel fails frequently when the request has already been started. + // In that case, we have to let it complete and check the canceled bool + // instead. The cancelation callback will delete the AssetRequest object. + uv_cancel((uv_req_t *)&impl->req); +} + } diff --git a/platform/default/asset_request_zip.cpp b/platform/default/asset_request_zip.cpp index 69de1c742..9f7ed6d9b 100644 --- a/platform/default/asset_request_zip.cpp +++ b/platform/default/asset_request_zip.cpp @@ -77,7 +77,7 @@ class AssetRequestImpl { MBGL_STORE_THREAD(tid) public: - AssetRequestImpl(AssetRequest *request, uv_loop_t *loop); + AssetRequestImpl(AssetRequest*, uv_loop_t*, const std::string& assetRoot); ~AssetRequestImpl(); void cancel(); @@ -112,10 +112,10 @@ AssetRequestImpl::~AssetRequestImpl() { } } -AssetRequestImpl::AssetRequestImpl(AssetRequest *request_, uv_loop_t *loop) +AssetRequestImpl::AssetRequestImpl(AssetRequest* request_, uv_loop_t* loop, const std::string& assetRoot_) : context(*AssetZipContext::Get(loop)), request(request_), - root(request->assetRoot), + root(assetRoot_), path(std::string { "assets/" } + request->resource.url.substr(8)) { auto zip = context.getHandle(root); if (zip) { @@ -275,30 +275,22 @@ void AssetRequestImpl::cancel() { // ------------------------------------------------------------------------------------------------- -AssetRequest::AssetRequest(const Resource& resource_, Callback callback_, const std::string& assetRoot_) +AssetRequest::AssetRequest(const Resource& resource_, Callback callback_, + uv_loop_t* loop, const std::string& assetRoot) : RequestBase(resource_, callback_) - , assetRoot(assetRoot_) { + , impl(new AssetRequestImpl(this, loop, assetRoot)) { assert(algo::starts_with(resource.url, "asset://")); } AssetRequest::~AssetRequest() { - if (ptr) { - reinterpret_cast(ptr)->cancel(); + if (impl) { + impl->cancel(); } } -void AssetRequest::start(uv_loop_t *loop, std::shared_ptr response) { - // We're ignoring the existing response if any. - (void(response)); - - assert(!ptr); - ptr = new AssetRequestImpl(this, loop); - // Note: the AssetRequestImpl deletes itself. -} - void AssetRequest::cancel() { - if (ptr) { - reinterpret_cast(ptr)->cancel(); + if (impl) { + impl->cancel(); } delete this; diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp index ffe3b525a..95cc88211 100644 --- a/platform/default/http_request_curl.cpp +++ b/platform/default/http_request_curl.cpp @@ -495,7 +495,7 @@ HTTPRequestImpl::~HTTPRequestImpl() { if (request) { context->removeRequest(request); - request->ptr = nullptr; + request->impl = nullptr; } handleError(curl_multi_remove_handle(context->multi, handle)); @@ -723,33 +723,27 @@ void HTTPRequestImpl::handleResult(CURLcode code) { // ------------------------------------------------------------------------------------------------- -HTTPRequest::HTTPRequest(const Resource& resource_, Callback callback_) - : RequestBase(resource_, callback_) { +HTTPRequest::HTTPRequest(const Resource& resource_, Callback callback_, + uv_loop_t* loop, std::shared_ptr response) + : RequestBase(resource_, callback_) + , impl(new HTTPRequestImpl(this, loop, response)) { } HTTPRequest::~HTTPRequest() { - if (ptr) { - reinterpret_cast(ptr)->abandon(); + if (impl) { + impl->abandon(); } } -void HTTPRequest::start(uv_loop_t *loop, std::shared_ptr response) { - assert(!ptr); - ptr = new HTTPRequestImpl(this, loop, response); -} - void HTTPRequest::retryImmediately() { - if (ptr) { - reinterpret_cast(ptr)->retryImmediately(); + if (impl) { + impl->retryImmediately(); } } void HTTPRequest::cancel() { - if (ptr) { - delete reinterpret_cast(ptr); - ptr = nullptr; - } - + delete impl; + impl = nullptr; delete this; } diff --git a/src/mbgl/storage/asset_request.hpp b/src/mbgl/storage/asset_request.hpp index e20e1d818..3f9b018b5 100644 --- a/src/mbgl/storage/asset_request.hpp +++ b/src/mbgl/storage/asset_request.hpp @@ -3,20 +3,22 @@ #include "request_base.hpp" +typedef struct uv_loop_s uv_loop_t; + namespace mbgl { +class AssetRequestImpl; + class AssetRequest : public RequestBase { public: - AssetRequest(const Resource&, Callback, const std::string& assetRoot); + AssetRequest(const Resource&, Callback, uv_loop_t*, const std::string& assetRoot); - void start(uv_loop_t *loop, std::shared_ptr response = nullptr); - void cancel(); + void cancel() override; private: ~AssetRequest(); - void *ptr = nullptr; - const std::string& assetRoot; + AssetRequestImpl* impl; friend class AssetRequestImpl; }; diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index c510b5c7f..3e02c05fa 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -134,12 +134,10 @@ void DefaultFileSource::Impl::startRealRequest(const Resource& resource, std::sh }; if (algo::starts_with(resource.url, "asset://")) { - request->request = new AssetRequest(resource, callback, assetRoot); + request->request = new AssetRequest(resource, callback, request->loop, assetRoot); } else { - request->request = new HTTPRequest(resource, callback); + request->request = new HTTPRequest(resource, callback, request->loop, response); } - - request->request->start(request->loop, response); } void DefaultFileSource::Impl::cancel(Request* req) { diff --git a/src/mbgl/storage/http_request.hpp b/src/mbgl/storage/http_request.hpp index faf6ee2f1..bbe1265c6 100644 --- a/src/mbgl/storage/http_request.hpp +++ b/src/mbgl/storage/http_request.hpp @@ -3,23 +3,23 @@ #include "request_base.hpp" +typedef struct uv_loop_s uv_loop_t; + namespace mbgl { -struct Resource; +class HTTPRequestImpl; class HTTPRequest : public RequestBase { public: - HTTPRequest(const Resource&, Callback); - - void start(uv_loop_t *loop, std::shared_ptr response = nullptr); - void cancel(); + HTTPRequest(const Resource&, Callback, uv_loop_t*, std::shared_ptr = nullptr); + void cancel() override; void retryImmediately(); private: ~HTTPRequest(); - void *ptr = nullptr; + HTTPRequestImpl* impl; friend class HTTPRequestImpl; }; diff --git a/src/mbgl/storage/request_base.hpp b/src/mbgl/storage/request_base.hpp index 281e76a02..e4ea7117c 100644 --- a/src/mbgl/storage/request_base.hpp +++ b/src/mbgl/storage/request_base.hpp @@ -7,8 +7,6 @@ #include #include -typedef struct uv_loop_s uv_loop_t; - namespace mbgl { struct Resource; @@ -19,19 +17,16 @@ public: using Callback = std::function response, FileCache::Hint hint)>; RequestBase(const Resource& resource_, Callback notify_) - : notify(notify_) - , resource(resource_) { + : resource(resource_) + , notify(notify_) { } virtual ~RequestBase() = default; - - virtual void start(uv_loop_t *loop, std::shared_ptr response = nullptr) = 0; virtual void cancel() = 0; - Callback notify; - protected: const Resource& resource; + Callback notify; }; } From c0b75f1af77e78e309cb18807de74f3788488a7a Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 27 Apr 2015 13:05:30 -0700 Subject: [PATCH 9/9] Remove Environment::terminate and associated code Fixes #1336 --- include/mbgl/map/environment.hpp | 5 -- include/mbgl/storage/default_file_source.hpp | 9 +-- include/mbgl/storage/file_source.hpp | 14 +---- include/mbgl/storage/request.hpp | 8 +-- src/mbgl/map/environment.cpp | 10 +--- src/mbgl/storage/default_file_source.cpp | 41 +------------ src/mbgl/storage/default_file_source_impl.hpp | 1 - src/mbgl/storage/request.cpp | 4 +- test/fixtures/database/cache.db | Bin 0 -> 4096 bytes test/fixtures/database/invalid.db | Bin 0 -> 4096 bytes test/storage/cache_response.cpp | 5 +- test/storage/cache_revalidate.cpp | 14 ++--- test/storage/directory_reading.cpp | 6 +- test/storage/file_reading.cpp | 12 +--- test/storage/http_cancel.cpp | 9 +-- test/storage/http_coalescing.cpp | 4 +- test/storage/http_environment.cpp | 54 ------------------ test/storage/http_error.cpp | 6 +- test/storage/http_header_parsing.cpp | 6 +- test/storage/http_load.cpp | 4 +- test/storage/http_noloop.cpp | 4 +- test/storage/http_other_loop.cpp | 4 +- test/storage/http_reading.cpp | 14 ++--- test/test.gypi | 1 - 24 files changed, 42 insertions(+), 193 deletions(-) create mode 100644 test/fixtures/database/cache.db create mode 100644 test/fixtures/database/invalid.db delete mode 100644 test/storage/http_environment.cpp diff --git a/include/mbgl/map/environment.hpp b/include/mbgl/map/environment.hpp index 1cd33a5e6..a00fb2445 100644 --- a/include/mbgl/map/environment.hpp +++ b/include/mbgl/map/environment.hpp @@ -54,11 +54,6 @@ public: // Only call this while the OpenGL context is exclusive to this thread. void performCleanup(); - // ############################################################################################# - - // Request to terminate the environment. - void terminate(); - private: unsigned id; FileSource& fileSource; diff --git a/include/mbgl/storage/default_file_source.hpp b/include/mbgl/storage/default_file_source.hpp index f39374716..d50d5ddea 100644 --- a/include/mbgl/storage/default_file_source.hpp +++ b/include/mbgl/storage/default_file_source.hpp @@ -16,12 +16,9 @@ public: ~DefaultFileSource() override; // FileSource API - Request *request(const Resource &resource, uv_loop_t *loop, const Environment &env, - Callback callback) override; - void cancel(Request *request) override; - void request(const Resource &resource, const Environment &env, Callback callback) override; - - void abort(const Environment &env) override; + Request* request(const Resource&, uv_loop_t*, Callback) override; + void cancel(Request*) override; + void request(const Resource&, Callback) override; public: class Impl; diff --git a/include/mbgl/storage/file_source.hpp b/include/mbgl/storage/file_source.hpp index 30e88c39f..535c41e81 100644 --- a/include/mbgl/storage/file_source.hpp +++ b/include/mbgl/storage/file_source.hpp @@ -15,7 +15,6 @@ typedef struct uv_loop_s uv_loop_t; namespace mbgl { class Request; -class Environment; class FileSource : private util::noncopyable { protected: @@ -28,19 +27,12 @@ public: // These can be called from any thread. The callback will be invoked in the loop. // You can only cancel a request from the same thread it was created in. - virtual Request *request(const Resource &resource, uv_loop_t *loop, const Environment &env, - Callback callback) = 0; - virtual void cancel(Request *request) = 0; + virtual Request* request(const Resource&, uv_loop_t*, Callback) = 0; + virtual void cancel(Request*) = 0; // These can be called from any thread. The callback will be invoked in an arbitrary other thread. // You cannot cancel these requests. - virtual void request(const Resource &resource, const Environment &env, Callback callback) = 0; - - // This can be called from any thread. All requests with the environment pointer env should be - // notified as errored. Note that this is /different/ from canceling requests; a canceled - // request's callback is never called, while an aborted request's callback is called with - // a error message. - virtual void abort(const Environment &env) = 0; + virtual void request(const Resource&, Callback) = 0; }; } diff --git a/include/mbgl/storage/request.hpp b/include/mbgl/storage/request.hpp index 00157329b..a01808ce8 100644 --- a/include/mbgl/storage/request.hpp +++ b/include/mbgl/storage/request.hpp @@ -17,14 +17,13 @@ typedef struct uv_loop_s uv_loop_t; namespace mbgl { class Response; -class Environment; class Request : private util::noncopyable { MBGL_STORE_THREAD(tid) public: using Callback = std::function; - Request(const Resource &resource, uv_loop_t *loop, const Environment &env, Callback callback); + Request(const Resource &resource, uv_loop_t *loop, Callback callback); public: // May be called from any thread. @@ -48,11 +47,6 @@ private: public: const Resource resource; - - // The environment ref is used to associate requests with a particular environment. This allows - // us to only terminate requests associated with that environment, e.g. when the map the env - // belongs to is discarded. - const Environment &env; }; } diff --git a/src/mbgl/map/environment.cpp b/src/mbgl/map/environment.cpp index 469790501..4c7a5faf8 100644 --- a/src/mbgl/map/environment.cpp +++ b/src/mbgl/map/environment.cpp @@ -122,13 +122,13 @@ unsigned Environment::getID() const { void Environment::requestAsync(const Resource& resource, std::function callback) { - fileSource.request(resource, *this, std::move(callback)); + fileSource.request(resource, std::move(callback)); } Request* Environment::request(const Resource& resource, std::function callback) { assert(currentlyOn(ThreadType::Map)); - return fileSource.request(resource, loop, *this, std::move(callback)); + return fileSource.request(resource, loop, std::move(callback)); } void Environment::cancelRequest(Request* req) { @@ -178,10 +178,4 @@ void Environment::performCleanup() { } } -// ############################################################################################# - -void Environment::terminate() { - fileSource.abort(*this); -} - } diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 3e02c05fa..7080809d7 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -10,7 +10,6 @@ #include #include #include -#include #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wshadow" @@ -38,9 +37,8 @@ DefaultFileSource::~DefaultFileSource() { Request* DefaultFileSource::request(const Resource& resource, uv_loop_t* l, - const Environment& env, Callback callback) { - auto req = new Request(resource, l, env, std::move(callback)); + auto req = new Request(resource, l, std::move(callback)); // This function can be called from any thread. Make sure we're executing the actual call in the // file source loop by sending it over the queue. @@ -49,8 +47,8 @@ Request* DefaultFileSource::request(const Resource& resource, return req; } -void DefaultFileSource::request(const Resource& resource, const Environment& env, Callback callback) { - request(resource, nullptr, env, std::move(callback)); +void DefaultFileSource::request(const Resource& resource, Callback callback) { + request(resource, nullptr, std::move(callback)); } void DefaultFileSource::cancel(Request *req) { @@ -61,10 +59,6 @@ void DefaultFileSource::cancel(Request *req) { thread->invoke(&Impl::cancel, req); } -void DefaultFileSource::abort(const Environment &env) { - thread->invoke(&Impl::abort, std::ref(env)); -} - // ----- Impl ----- DefaultFileSource::Impl::Impl(FileCache* cache_, const std::string& root) @@ -163,35 +157,6 @@ void DefaultFileSource::Impl::cancel(Request* req) { req->destruct(); } -// Aborts all requests that are part of the current environment. -void DefaultFileSource::Impl::abort(const Environment& env) { - // Construct a cancellation response. - auto response = std::make_shared(); - response->status = Response::Error; - response->message = "Environment is terminating"; - - // Iterate through all pending requests and remove them in case they're abandoned. - util::erase_if(pending, [&](std::pair& it) -> bool { - // Notify all pending requests that are in the current environment. - util::erase_if(it.second.observers, [&](Request* req) -> bool { - if (&req->env == &env) { - req->notify(response); - return true; - } else { - return false; - } - }); - - bool abandoned = it.second.observers.empty(); - - if (abandoned && it.second.request) { - it.second.request->cancel(); - } - - return abandoned; - }); -} - void DefaultFileSource::Impl::notify(DefaultFileRequest* request, std::shared_ptr response, FileCache::Hint hint) { // First, remove the request, since it might be destructed at any point now. assert(find(request->resource) == request); diff --git a/src/mbgl/storage/default_file_source_impl.hpp b/src/mbgl/storage/default_file_source_impl.hpp index d3f83af38..1d4da0906 100644 --- a/src/mbgl/storage/default_file_source_impl.hpp +++ b/src/mbgl/storage/default_file_source_impl.hpp @@ -26,7 +26,6 @@ public: void add(Request*, uv_loop_t*); void cancel(Request*); - void abort(const Environment&); private: DefaultFileRequest* find(const Resource&); diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index ea80e5950..d413cabbe 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -16,8 +16,8 @@ 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_) { +Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_) + : callback(callback_), resource(resource_) { // 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) { diff --git a/test/fixtures/database/cache.db b/test/fixtures/database/cache.db new file mode 100644 index 0000000000000000000000000000000000000000..c358381f1f2df0b84c23809ec23b385c7d082388 GIT binary patch literal 4096 zcmeH`-A=+V6oA`p5o5xY7u=rRNur5{7jQ@=$Q(LH0#|EgMHayUD+#yi6ZsUriBI8$ zWdpVl@Bx-KY1;GC)6=i#JU!Z>;4qBl6DFXB41_Vd20#dDavE}O#;%MvhJRKKUHlxE z^QxT@T*^z8ncL^#^P);{K~t_+gM!88{$rx{5=qAE&rfF-`nA6YP1+v$NSwTZi|ox=nVXfE;u&A5-v1&w~fIb#J+S zXp=rT9S@wYZJ#GTv0!2mQ<UO7|%7bV!m0oc!+%^?xkXx&3dvIB;X-Ac&iBZBno{eQs`)nbiMD{Z@{#CZu wWf>afRW0VbypXl29;FB<0{aM@tdy$%_n}cbMWFBmRR0(Li`4@afrAA800yR#H~;_u literal 0 HcmV?d00001 diff --git a/test/fixtures/database/invalid.db b/test/fixtures/database/invalid.db new file mode 100644 index 0000000000000000000000000000000000000000..f07e4af03da4e33ef3ee4027f39c92c6a2ec2ab0 GIT binary patch literal 4096 zcmeH}K~KUk6oA{6=rAEM(S*d))m?7o$ieT3d!%{ zQ^)qO=k-N$LOkno$r)6vMkTg8f!|ZB!kKW>RG>y+X-Y$)4meir9xwT~*T# zAIKoGodk`upXgD2XVhE7}PUlV2%$S>|&wK4)cu*`#5z4CYgEe(u pOumq6S~LMoV7UpblYDya8xYsO`ad5n_r>+?GyzRuo&X_c`WrA$q!IuC literal 0 HcmV?d00001 diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp index d5dd8b36e..87de62025 100644 --- a/test/storage/cache_response.cpp +++ b/test/storage/cache_response.cpp @@ -14,9 +14,8 @@ TEST_F(Storage, CacheResponse) { DefaultFileSource fs(&cache); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/cache" }; - auto &env = *static_cast(nullptr); - fs.request(resource, uv_default_loop(), env, [&](const Response &res) { + fs.request(resource, uv_default_loop(), [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response 1", res.data); EXPECT_LT(0, res.expires); @@ -24,7 +23,7 @@ TEST_F(Storage, CacheResponse) { EXPECT_EQ("", res.etag); EXPECT_EQ("", res.message); - fs.request(resource, uv_default_loop(), env, [&, res](const Response &res2) { + fs.request(resource, uv_default_loop(), [&, res](const Response &res2) { EXPECT_EQ(res.status, res2.status); EXPECT_EQ(res.data, res2.data); EXPECT_EQ(res.expires, res2.expires); diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp index ceb6b2969..1bfd6941b 100644 --- a/test/storage/cache_revalidate.cpp +++ b/test/storage/cache_revalidate.cpp @@ -15,10 +15,8 @@ TEST_F(Storage, CacheRevalidate) { SQLiteCache cache(":memory:"); DefaultFileSource fs(&cache); - auto &env = *static_cast(nullptr); - const Resource revalidateSame { Resource::Unknown, "http://127.0.0.1:3000/revalidate-same" }; - fs.request(revalidateSame, uv_default_loop(), env, [&](const Response &res) { + fs.request(revalidateSame, uv_default_loop(), [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response", res.data); EXPECT_EQ(0, res.expires); @@ -26,7 +24,7 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("snowfall", res.etag); EXPECT_EQ("", res.message); - fs.request(revalidateSame, uv_default_loop(), env, [&, res](const Response &res2) { + fs.request(revalidateSame, uv_default_loop(), [&, res](const Response &res2) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response", res2.data); // We use this to indicate that a 304 reply came back. @@ -42,7 +40,7 @@ TEST_F(Storage, CacheRevalidate) { const Resource revalidateModified{ Resource::Unknown, "http://127.0.0.1:3000/revalidate-modified" }; - fs.request(revalidateModified, uv_default_loop(), env, [&](const Response &res) { + fs.request(revalidateModified, uv_default_loop(), [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response", res.data); EXPECT_EQ(0, res.expires); @@ -50,7 +48,7 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("", res.etag); EXPECT_EQ("", res.message); - fs.request(revalidateModified, uv_default_loop(), env, [&, res](const Response &res2) { + fs.request(revalidateModified, uv_default_loop(), [&, res](const Response &res2) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response", res2.data); // We use this to indicate that a 304 reply came back. @@ -64,7 +62,7 @@ TEST_F(Storage, CacheRevalidate) { }); const Resource revalidateEtag { Resource::Unknown, "http://127.0.0.1:3000/revalidate-etag" }; - fs.request(revalidateEtag, uv_default_loop(), env, [&](const Response &res) { + fs.request(revalidateEtag, uv_default_loop(), [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response 1", res.data); EXPECT_EQ(0, res.expires); @@ -72,7 +70,7 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("response-1", res.etag); EXPECT_EQ("", res.message); - fs.request(revalidateEtag, uv_default_loop(), env, [&, res](const Response &res2) { + fs.request(revalidateEtag, uv_default_loop(), [&, res](const Response &res2) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response 2", res2.data); EXPECT_EQ(0, res2.expires); diff --git a/test/storage/directory_reading.cpp b/test/storage/directory_reading.cpp index ccae4177c..4459b42c2 100644 --- a/test/storage/directory_reading.cpp +++ b/test/storage/directory_reading.cpp @@ -15,10 +15,8 @@ TEST_F(Storage, AssetReadDirectory) { DefaultFileSource fs(nullptr); #endif - auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage" }, uv_default_loop(), - env, [&](const Response &res) { + [&](const Response &res) { EXPECT_EQ(Response::Error, res.status); EXPECT_EQ(0ul, res.data.size()); EXPECT_EQ(0, res.expires); @@ -27,7 +25,7 @@ TEST_F(Storage, AssetReadDirectory) { #ifdef MBGL_ASSET_ZIP EXPECT_EQ("No such file", res.message); #elif MBGL_ASSET_FS - EXPECT_EQ("illegal operation on a directory", res.message); + EXPECT_EQ("illegal operation on a directory", res.message); #endif ReadDirectory.finish(); }); diff --git a/test/storage/file_reading.cpp b/test/storage/file_reading.cpp index 01db75d5c..8db911f65 100644 --- a/test/storage/file_reading.cpp +++ b/test/storage/file_reading.cpp @@ -16,10 +16,8 @@ TEST_F(Storage, AssetEmptyFile) { DefaultFileSource fs(nullptr); #endif - auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/empty" }, uv_default_loop(), - env, [&](const Response &res) { + [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(0ul, res.data.size()); EXPECT_EQ(0, res.expires); @@ -43,10 +41,8 @@ TEST_F(Storage, AssetNonEmptyFile) { DefaultFileSource fs(nullptr); #endif - auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/nonempty" }, - uv_default_loop(), env, [&](const Response &res) { + uv_default_loop(), [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(16ul, res.data.size()); EXPECT_EQ(0, res.expires); @@ -71,10 +67,8 @@ TEST_F(Storage, AssetNonExistentFile) { DefaultFileSource fs(nullptr); #endif - auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/does_not_exist" }, - uv_default_loop(), env, [&](const Response &res) { + uv_default_loop(), [&](const Response &res) { EXPECT_EQ(Response::Error, res.status); EXPECT_EQ(0ul, res.data.size()); EXPECT_EQ(0, res.expires); diff --git a/test/storage/http_cancel.cpp b/test/storage/http_cancel.cpp index 80efb3977..a73182da5 100644 --- a/test/storage/http_cancel.cpp +++ b/test/storage/http_cancel.cpp @@ -14,10 +14,8 @@ TEST_F(Storage, HTTPCancel) { DefaultFileSource fs(nullptr); - auto &env = *static_cast(nullptr); - auto req = - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), [&](const Response &) { ADD_FAILURE() << "Callback should not be called"; }); fs.cancel(req); @@ -33,13 +31,12 @@ TEST_F(Storage, HTTPCancelMultiple) { DefaultFileSource fs(nullptr); - auto &env = *static_cast(nullptr); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; - auto req2 = fs.request(resource, uv_default_loop(), env, [&](const Response &) { + auto req2 = fs.request(resource, uv_default_loop(), [&](const Response &) { ADD_FAILURE() << "Callback should not be called"; }); - fs.request(resource, uv_default_loop(), env, [&](const Response &res) { + fs.request(resource, uv_default_loop(), [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(0, res.expires); diff --git a/test/storage/http_coalescing.cpp b/test/storage/http_coalescing.cpp index 28fa4415b..5c90009f8 100644 --- a/test/storage/http_coalescing.cpp +++ b/test/storage/http_coalescing.cpp @@ -14,8 +14,6 @@ TEST_F(Storage, HTTPCoalescing) { DefaultFileSource fs(nullptr); - auto &env = *static_cast(nullptr); - static const Response *reference = nullptr; const auto complete = [&](const Response &res) { @@ -43,7 +41,7 @@ TEST_F(Storage, HTTPCoalescing) { const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; for (int i = 0; i < total; i++) { - fs.request(resource, uv_default_loop(), env, complete); + fs.request(resource, uv_default_loop(), complete); } uv_run(uv_default_loop(), UV_RUN_DEFAULT); diff --git a/test/storage/http_environment.cpp b/test/storage/http_environment.cpp deleted file mode 100644 index 41a9d43d5..000000000 --- a/test/storage/http_environment.cpp +++ /dev/null @@ -1,54 +0,0 @@ -#include "storage.hpp" - -#include - -#include -#include - -#include - -TEST_F(Storage, HTTPCancelEnvironment) { - SCOPED_TEST(HTTPRetainedEnvironment) - SCOPED_TEST(HTTPCanceledEnvironment) - - using namespace mbgl; - - DefaultFileSource fs(nullptr); - - // Create two fake environment pointers. The FileSource implementation treats these as opaque - // pointers and doesn't reach into them. - auto &env1 = *reinterpret_cast(1); - auto &env2 = *reinterpret_cast(2); - - const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/delayed" }; - - // Environment 1 - fs.request(resource, uv_default_loop(), env1, [&](const Response &res) { - // This environment gets aborted below. This means the request is marked as failing and - // will return an error here. - EXPECT_EQ(Response::Error, res.status); - EXPECT_EQ("", res.data); - EXPECT_EQ(0, res.expires); - EXPECT_EQ(0, res.modified); - EXPECT_EQ("", res.etag); - EXPECT_EQ("Environment is terminating", res.message); - HTTPCanceledEnvironment.finish(); - }); - - // Environment 2 - fs.request(resource, uv_default_loop(), env2, [&](const Response &res) { - // The same request as above, but in a different environment which doesn't get aborted. This - // means the request should succeed. - EXPECT_EQ(Response::Successful, res.status); - EXPECT_EQ("Response", res.data); - EXPECT_EQ(0, res.expires); - EXPECT_EQ(0, res.modified); - EXPECT_EQ("", res.etag); - EXPECT_EQ("", res.message); - HTTPRetainedEnvironment.finish(); - }); - - fs.abort(env1); - - uv_run(uv_default_loop(), UV_RUN_DEFAULT); -} diff --git a/test/storage/http_error.cpp b/test/storage/http_error.cpp index e5728d97b..eeb8117a0 100644 --- a/test/storage/http_error.cpp +++ b/test/storage/http_error.cpp @@ -22,12 +22,10 @@ TEST_F(Storage, HTTPError) { DefaultFileSource fs(nullptr); - auto &env = *static_cast(nullptr); - auto start = uv_hrtime(); fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, uv_default_loop(), - env, [&](const Response &res) { + [&](const Response &res) { const auto duration = double(uv_hrtime() - start) / 1e9; EXPECT_LT(1, duration) << "Backoff timer didn't wait 1 second"; EXPECT_GT(1.2, duration) << "Backoff timer fired too late"; @@ -41,7 +39,7 @@ TEST_F(Storage, HTTPError) { HTTPTemporaryError.finish(); }); - fs.request({ Resource::Unknown, "http://127.0.0.1:3001/" }, uv_default_loop(), env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3001/" }, uv_default_loop(), [&](const Response &res) { const auto duration = double(uv_hrtime() - start) / 1e9; // 1.5 seconds == 4 retries, with a 500ms timeout (see above). diff --git a/test/storage/http_header_parsing.cpp b/test/storage/http_header_parsing.cpp index 1561660b6..5f18b163a 100644 --- a/test/storage/http_header_parsing.cpp +++ b/test/storage/http_header_parsing.cpp @@ -15,11 +15,9 @@ TEST_F(Storage, HTTPHeaderParsing) { DefaultFileSource fs(nullptr); - auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test?modified=1420794326&expires=1420797926&etag=foo" }, - uv_default_loop(), env, [&](const Response &res) { + uv_default_loop(), [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(1420797926, res.expires); @@ -33,7 +31,7 @@ TEST_F(Storage, HTTPHeaderParsing) { SystemClock::now().time_since_epoch()).count(); fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=120" }, - uv_default_loop(), env, [&](const Response &res) { + uv_default_loop(), [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); EXPECT_GT(2, std::abs(res.expires - now - 120)) << "Expiration date isn't about 120 seconds in the future"; diff --git a/test/storage/http_load.cpp b/test/storage/http_load.cpp index c4069eba3..2347a76ab 100644 --- a/test/storage/http_load.cpp +++ b/test/storage/http_load.cpp @@ -11,8 +11,6 @@ TEST_F(Storage, HTTPLoad) { DefaultFileSource fs(nullptr); - auto &env = *static_cast(nullptr); - const int concurrency = 50; const int max = 10000; int number = 1; @@ -21,7 +19,7 @@ TEST_F(Storage, HTTPLoad) { const auto current = number++; fs.request({ Resource::Unknown, std::string("http://127.0.0.1:3000/load/") + std::to_string(current) }, - uv_default_loop(), env, [&, current](const Response &res) { + uv_default_loop(), [&, current](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(std::string("Request ") + std::to_string(current), res.data); EXPECT_EQ(0, res.expires); diff --git a/test/storage/http_noloop.cpp b/test/storage/http_noloop.cpp index cd4ebeb2c..e133d3e0c 100644 --- a/test/storage/http_noloop.cpp +++ b/test/storage/http_noloop.cpp @@ -12,8 +12,6 @@ TEST_F(Storage, HTTPNoLoop) { DefaultFileSource fs(nullptr); - auto &env = *static_cast(nullptr); - const auto mainThread = uv_thread_self(); // Async handle that keeps the main loop alive until the thread finished @@ -22,7 +20,7 @@ TEST_F(Storage, HTTPNoLoop) { uv::close(as); }); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, nullptr, env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, nullptr, [&](const Response &res) { EXPECT_NE(uv_thread_self(), mainThread) << "Response was called in the same thread"; EXPECT_EQ(Response::Successful, res.status); diff --git a/test/storage/http_other_loop.cpp b/test/storage/http_other_loop.cpp index 9ad673cf7..64612f13d 100644 --- a/test/storage/http_other_loop.cpp +++ b/test/storage/http_other_loop.cpp @@ -12,9 +12,7 @@ TEST_F(Storage, HTTPOtherLoop) { // This file source launches a separate thread to do the processing. DefaultFileSource fs(nullptr); - auto &env = *static_cast(nullptr); - - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp index 350a8eaa4..09d33fd6a 100644 --- a/test/storage/http_reading.cpp +++ b/test/storage/http_reading.cpp @@ -14,11 +14,9 @@ TEST_F(Storage, HTTPReading) { DefaultFileSource fs(nullptr); - auto &env = *static_cast(nullptr); - const auto mainThread = uv_thread_self(); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), [&](const Response &res) { EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Successful, res.status); @@ -31,7 +29,7 @@ TEST_F(Storage, HTTPReading) { }); fs.request({ Resource::Unknown, "http://127.0.0.1:3000/doesnotexist" }, uv_default_loop(), - env, [&](const Response &res) { + [&](const Response &res) { EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Error, res.status); EXPECT_EQ("HTTP status code 404", res.message); @@ -51,9 +49,7 @@ TEST_F(Storage, HTTPNoCallback) { DefaultFileSource fs(nullptr); - auto &env = *static_cast(nullptr); - - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), nullptr); uv_run(uv_default_loop(), UV_RUN_DEFAULT); @@ -68,10 +64,8 @@ TEST_F(Storage, HTTPCallbackNotOnLoop) { SCOPED_TEST(HTTPTest) - auto &env = *static_cast(nullptr); - std::promise promise; - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, env, [&] (const Response &) { + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, [&] (const Response &) { promise.set_value(); }); diff --git a/test/test.gypi b/test/test.gypi index 5c975d547..d22dc3b43 100644 --- a/test/test.gypi +++ b/test/test.gypi @@ -62,7 +62,6 @@ 'storage/file_reading.cpp', 'storage/http_cancel.cpp', 'storage/http_coalescing.cpp', - 'storage/http_environment.cpp', 'storage/http_error.cpp', 'storage/http_header_parsing.cpp', 'storage/http_load.cpp',