From 94a5ac857f7cfc0a8e20035fabc39841fe17249c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Thu, 15 Oct 2015 15:10:41 +0200 Subject: [PATCH 1/8] [core] all requests have to be canceled explicitly now By not automatically destroying Request objects after the result has been delivered, we are making sure that we can potentially fire the callback multiple times without adverse effects. This means that you have to hold on to the result of fs->request(), can explicitly cancel it if you don't want to be notified of data changes anymore. Not doing so will monitor the request indefinitely and will prevent the app from exiting. --- platform/node/src/node_file_source.cpp | 1 - src/mbgl/map/map_context.cpp | 5 +++-- src/mbgl/map/raster_tile_data.cpp | 1 + src/mbgl/map/source.cpp | 2 ++ src/mbgl/map/sprite.cpp | 4 ++++ src/mbgl/map/vector_tile_data.cpp | 1 + src/mbgl/storage/default_file_source.cpp | 6 ++++++ src/mbgl/storage/request.cpp | 10 +++++----- src/mbgl/text/glyph_pbf.cpp | 2 ++ test/storage/cache_response.cpp | 6 ++++-- test/storage/cache_revalidate.cpp | 18 ++++++++++++------ test/storage/database.cpp | 8 ++++---- test/storage/directory_reading.cpp | 3 ++- test/storage/file_reading.cpp | 9 ++++++--- test/storage/http_cancel.cpp | 3 ++- test/storage/http_coalescing.cpp | 6 +++++- test/storage/http_error.cpp | 6 ++++-- test/storage/http_header_parsing.cpp | 6 ++++-- test/storage/http_issue_1369.cpp | 3 ++- test/storage/http_load.cpp | 14 +++++++++----- test/storage/http_other_loop.cpp | 3 ++- test/storage/http_reading.cpp | 20 ++++++++++++++------ 22 files changed, 94 insertions(+), 43 deletions(-) diff --git a/platform/node/src/node_file_source.cpp b/platform/node/src/node_file_source.cpp index b4828e7687a..5a5a34f0ce1 100644 --- a/platform/node/src/node_file_source.cpp +++ b/platform/node/src/node_file_source.cpp @@ -133,7 +133,6 @@ void NodeFileSource::notify(const mbgl::Resource& resource, const std::shared_pt } observersIt->second->notify(response); - observers.erase(observersIt); } } diff --git a/src/mbgl/map/map_context.cpp b/src/mbgl/map/map_context.cpp index d3324cb2772..573882da87e 100644 --- a/src/mbgl/map/map_context.cpp +++ b/src/mbgl/map/map_context.cpp @@ -53,8 +53,7 @@ void MapContext::cleanup() { view.notify(); if (styleRequest) { - FileSource* fs = util::ThreadContext::getFileSource(); - fs->cancel(styleRequest); + util::ThreadContext::getFileSource()->cancel(styleRequest); styleRequest = nullptr; } @@ -100,6 +99,7 @@ void MapContext::setStyleURL(const std::string& url) { if (styleRequest) { fs->cancel(styleRequest); + styleRequest = nullptr; } styleURL = url; @@ -114,6 +114,7 @@ void MapContext::setStyleURL(const std::string& url) { } styleRequest = fs->request({ Resource::Kind::Style, styleURL }, util::RunLoop::getLoop(), [this, base](const Response &res) { + util::ThreadContext::getFileSource()->cancel(styleRequest); styleRequest = nullptr; if (res.status == Response::Successful) { diff --git a/src/mbgl/map/raster_tile_data.cpp b/src/mbgl/map/raster_tile_data.cpp index 27094510a6d..c983a573752 100644 --- a/src/mbgl/map/raster_tile_data.cpp +++ b/src/mbgl/map/raster_tile_data.cpp @@ -31,6 +31,7 @@ void RasterTileData::request(float pixelRatio, FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Tile, url }, util::RunLoop::getLoop(), [url, callback, this](const Response &res) { + util::ThreadContext::getFileSource()->cancel(req); req = nullptr; if (res.status == Response::NotFound) { diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index 49e1d816104..3e8aba8859b 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -125,6 +125,7 @@ Source::Source() {} Source::~Source() { if (req) { util::ThreadContext::getFileSource()->cancel(req); + req = nullptr; } } @@ -153,6 +154,7 @@ void Source::load() { FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Source, info.url }, util::RunLoop::getLoop(), [this](const Response &res) { + util::ThreadContext::getFileSource()->cancel(req); req = nullptr; if (res.status != Response::Successful) { diff --git a/src/mbgl/map/sprite.cpp b/src/mbgl/map/sprite.cpp index d5628b05b27..02bf0895c14 100644 --- a/src/mbgl/map/sprite.cpp +++ b/src/mbgl/map/sprite.cpp @@ -29,9 +29,11 @@ struct Sprite::Loader { ~Loader() { if (jsonRequest) { util::ThreadContext::getFileSource()->cancel(jsonRequest); + jsonRequest = nullptr; } if (spriteRequest) { util::ThreadContext::getFileSource()->cancel(spriteRequest); + spriteRequest = nullptr; } } }; @@ -52,6 +54,7 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) FileSource* fs = util::ThreadContext::getFileSource(); loader->jsonRequest = fs->request({ Resource::Kind::SpriteJSON, jsonURL }, util::RunLoop::getLoop(), [this, jsonURL](const Response& res) { + util::ThreadContext::getFileSource()->cancel(loader->jsonRequest); loader->jsonRequest = nullptr; if (res.status == Response::Successful) { loader->data->json = res.data; @@ -68,6 +71,7 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) loader->spriteRequest = fs->request({ Resource::Kind::SpriteImage, spriteURL }, util::RunLoop::getLoop(), [this, spriteURL](const Response& res) { + util::ThreadContext::getFileSource()->cancel(loader->spriteRequest); loader->spriteRequest = nullptr; if (res.status == Response::Successful) { loader->data->image = res.data; diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp index 3fe96599d6c..a10dd64bd53 100644 --- a/src/mbgl/map/vector_tile_data.cpp +++ b/src/mbgl/map/vector_tile_data.cpp @@ -43,6 +43,7 @@ void VectorTileData::request(float pixelRatio, const std::function& call FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Tile, url }, util::RunLoop::getLoop(), [url, callback, this](const Response &res) { + util::ThreadContext::getFileSource()->cancel(req); req = nullptr; if (res.status == Response::NotFound) { diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index c33728db15f..7b3cff82533 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wshadow" @@ -40,6 +41,10 @@ Request* DefaultFileSource::request(const Resource& resource, Callback callback) { assert(l); + if (!callback) { + throw util::MisuseException("FileSource callback can't be empty"); + } + std::string url; switch (resource.kind) { @@ -70,6 +75,7 @@ Request* DefaultFileSource::request(const Resource& resource, } void DefaultFileSource::cancel(Request *req) { + assert(req); req->cancel(); thread->invoke(&Impl::cancel, req); } diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index 79d441442d8..f8ad555379a 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -6,6 +6,7 @@ #include #include +#include namespace mbgl { @@ -40,18 +41,17 @@ void Request::invoke() { // 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); + callback(*std::atomic_load(&response)); } - delete this; } Request::~Request() = default; // Called in the FileSource thread. void Request::notify(const std::shared_ptr &response_) { - assert(!response); - response = response_; - assert(response); + assert(!std::atomic_load(&response)); + assert(response_); + std::atomic_store(&response, response_); async->send(); } diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp index ba92ce6b4e9..f9f8afcb1bd 100644 --- a/src/mbgl/text/glyph_pbf.cpp +++ b/src/mbgl/text/glyph_pbf.cpp @@ -75,6 +75,7 @@ GlyphPBF::GlyphPBF(GlyphStore* store, }); auto requestCallback = [this, store, fontStack, url](const Response &res) { + util::ThreadContext::getFileSource()->cancel(req); req = nullptr; if (res.status != Response::Successful) { @@ -94,6 +95,7 @@ GlyphPBF::GlyphPBF(GlyphStore* store, GlyphPBF::~GlyphPBF() { if (req) { util::ThreadContext::getFileSource()->cancel(req); + req = nullptr; } } diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp index 87de62025e5..0fba2ba5e77 100644 --- a/test/storage/cache_response.cpp +++ b/test/storage/cache_response.cpp @@ -15,7 +15,8 @@ TEST_F(Storage, CacheResponse) { const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/cache" }; - fs.request(resource, uv_default_loop(), [&](const Response &res) { + Request* req = fs.request(resource, uv_default_loop(), [&](const Response &res) { + fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response 1", res.data); EXPECT_LT(0, res.expires); @@ -23,7 +24,8 @@ TEST_F(Storage, CacheResponse) { EXPECT_EQ("", res.etag); EXPECT_EQ("", res.message); - fs.request(resource, uv_default_loop(), [&, res](const Response &res2) { + req = fs.request(resource, uv_default_loop(), [&, res](const Response &res2) { + fs.cancel(req); 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 1bfd6941b75..8ebedef01b7 100644 --- a/test/storage/cache_revalidate.cpp +++ b/test/storage/cache_revalidate.cpp @@ -16,7 +16,8 @@ TEST_F(Storage, CacheRevalidate) { DefaultFileSource fs(&cache); const Resource revalidateSame { Resource::Unknown, "http://127.0.0.1:3000/revalidate-same" }; - fs.request(revalidateSame, uv_default_loop(), [&](const Response &res) { + Request* req = fs.request(revalidateSame, uv_default_loop(), [&](const Response &res) { + fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response", res.data); EXPECT_EQ(0, res.expires); @@ -24,7 +25,8 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("snowfall", res.etag); EXPECT_EQ("", res.message); - fs.request(revalidateSame, uv_default_loop(), [&, res](const Response &res2) { + req = fs.request(revalidateSame, uv_default_loop(), [&, res](const Response &res2) { + fs.cancel(req); EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response", res2.data); // We use this to indicate that a 304 reply came back. @@ -40,7 +42,8 @@ TEST_F(Storage, CacheRevalidate) { const Resource revalidateModified{ Resource::Unknown, "http://127.0.0.1:3000/revalidate-modified" }; - fs.request(revalidateModified, uv_default_loop(), [&](const Response &res) { + Request* req2 = fs.request(revalidateModified, uv_default_loop(), [&](const Response &res) { + fs.cancel(req2); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response", res.data); EXPECT_EQ(0, res.expires); @@ -48,7 +51,8 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("", res.etag); EXPECT_EQ("", res.message); - fs.request(revalidateModified, uv_default_loop(), [&, res](const Response &res2) { + req2 = fs.request(revalidateModified, uv_default_loop(), [&, res](const Response &res2) { + fs.cancel(req2); EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response", res2.data); // We use this to indicate that a 304 reply came back. @@ -62,7 +66,8 @@ TEST_F(Storage, CacheRevalidate) { }); const Resource revalidateEtag { Resource::Unknown, "http://127.0.0.1:3000/revalidate-etag" }; - fs.request(revalidateEtag, uv_default_loop(), [&](const Response &res) { + Request* req3 = fs.request(revalidateEtag, uv_default_loop(), [&](const Response &res) { + fs.cancel(req3); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response 1", res.data); EXPECT_EQ(0, res.expires); @@ -70,7 +75,8 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("response-1", res.etag); EXPECT_EQ("", res.message); - fs.request(revalidateEtag, uv_default_loop(), [&, res](const Response &res2) { + req3 = fs.request(revalidateEtag, uv_default_loop(), [&, res](const Response &res2) { + fs.cancel(req3); EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response 2", res2.data); EXPECT_EQ(0, res2.expires); diff --git a/test/storage/database.cpp b/test/storage/database.cpp index b40474c0040..20f96305b13 100644 --- a/test/storage/database.cpp +++ b/test/storage/database.cpp @@ -181,7 +181,7 @@ TEST_F(Storage, DatabaseLockedWrite) { response->data = "Demo"; cache.put({ Resource::Unknown, "mapbox://test" }, response); cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr res) { - EXPECT_NE(nullptr, res.get()); + ASSERT_NE(nullptr, res.get()); EXPECT_EQ("Demo", res->data); }); @@ -258,7 +258,7 @@ TEST_F(Storage, DatabaseDeleted) { response->data = "Demo"; cache.put({ Resource::Unknown, "mapbox://test" }, response); cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr res) { - EXPECT_NE(nullptr, res.get()); + ASSERT_NE(nullptr, res.get()); EXPECT_EQ("Demo", res->data); }); @@ -275,7 +275,7 @@ TEST_F(Storage, DatabaseDeleted) { response->data = "Demo"; cache.put({ Resource::Unknown, "mapbox://test" }, response); cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr res) { - EXPECT_NE(nullptr, res.get()); + ASSERT_NE(nullptr, res.get()); EXPECT_EQ("Demo", res->data); }); @@ -305,7 +305,7 @@ TEST_F(Storage, DatabaseInvalid) { response->data = "Demo"; cache.put({ Resource::Unknown, "mapbox://test" }, response); cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr res) { - EXPECT_NE(nullptr, res.get()); + ASSERT_NE(nullptr, res.get()); EXPECT_EQ("Demo", res->data); }); diff --git a/test/storage/directory_reading.cpp b/test/storage/directory_reading.cpp index 4459b42c2eb..11298dff8b8 100644 --- a/test/storage/directory_reading.cpp +++ b/test/storage/directory_reading.cpp @@ -15,8 +15,9 @@ TEST_F(Storage, AssetReadDirectory) { DefaultFileSource fs(nullptr); #endif - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage" }, uv_default_loop(), + Request* req = fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req); EXPECT_EQ(Response::Error, res.status); EXPECT_EQ(0ul, res.data.size()); EXPECT_EQ(0, res.expires); diff --git a/test/storage/file_reading.cpp b/test/storage/file_reading.cpp index 8db911f65e1..dc0620b78b6 100644 --- a/test/storage/file_reading.cpp +++ b/test/storage/file_reading.cpp @@ -16,8 +16,9 @@ TEST_F(Storage, AssetEmptyFile) { DefaultFileSource fs(nullptr); #endif - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/empty" }, uv_default_loop(), + Request* req = fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/empty" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(0ul, res.data.size()); EXPECT_EQ(0, res.expires); @@ -41,8 +42,9 @@ TEST_F(Storage, AssetNonEmptyFile) { DefaultFileSource fs(nullptr); #endif - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/nonempty" }, + Request* req = fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/nonempty" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(16ul, res.data.size()); EXPECT_EQ(0, res.expires); @@ -67,8 +69,9 @@ TEST_F(Storage, AssetNonExistentFile) { DefaultFileSource fs(nullptr); #endif - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/does_not_exist" }, + Request* req = fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/does_not_exist" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req); 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 a73182da559..2eb599eeaa5 100644 --- a/test/storage/http_cancel.cpp +++ b/test/storage/http_cancel.cpp @@ -36,7 +36,8 @@ TEST_F(Storage, HTTPCancelMultiple) { auto req2 = fs.request(resource, uv_default_loop(), [&](const Response &) { ADD_FAILURE() << "Callback should not be called"; }); - fs.request(resource, uv_default_loop(), [&](const Response &res) { + Request* req = fs.request(resource, uv_default_loop(), [&](const Response &res) { + fs.cancel(req); 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 5c90009f845..9dfa855501b 100644 --- a/test/storage/http_coalescing.cpp +++ b/test/storage/http_coalescing.cpp @@ -40,8 +40,12 @@ TEST_F(Storage, HTTPCoalescing) { const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; + Request* reqs[total]; for (int i = 0; i < total; i++) { - fs.request(resource, uv_default_loop(), complete); + reqs[i] = fs.request(resource, uv_default_loop(), [&complete, &fs, &reqs, i] (const Response &res) { + fs.cancel(reqs[i]); + complete(res); + }); } uv_run(uv_default_loop(), UV_RUN_DEFAULT); diff --git a/test/storage/http_error.cpp b/test/storage/http_error.cpp index 7edce61e5ca..aebc75405d5 100644 --- a/test/storage/http_error.cpp +++ b/test/storage/http_error.cpp @@ -30,8 +30,9 @@ TEST_F(Storage, HTTPError) { auto start = uv_hrtime(); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, uv_default_loop(), + Request* req1 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req1); 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"; @@ -45,8 +46,9 @@ TEST_F(Storage, HTTPError) { HTTPTemporaryError.finish(); }); - fs.request({ Resource::Unknown, "http://127.0.0.1:3001/" }, uv_default_loop(), + Request* req2 = fs.request({ Resource::Unknown, "http://127.0.0.1:3001/" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req2); const auto duration = double(uv_hrtime() - start) / 1e9; // 1.5 seconds == 4 retries, with a 500ms timeout (see above). EXPECT_LT(1.5, duration) << "Resource wasn't retried the correct number of times"; diff --git a/test/storage/http_header_parsing.cpp b/test/storage/http_header_parsing.cpp index 5f18b163a47..13eab991255 100644 --- a/test/storage/http_header_parsing.cpp +++ b/test/storage/http_header_parsing.cpp @@ -15,9 +15,10 @@ TEST_F(Storage, HTTPHeaderParsing) { DefaultFileSource fs(nullptr); - fs.request({ Resource::Unknown, + Request* req1 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test?modified=1420794326&expires=1420797926&etag=foo" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req1); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(1420797926, res.expires); @@ -30,8 +31,9 @@ TEST_F(Storage, HTTPHeaderParsing) { int64_t now = std::chrono::duration_cast( SystemClock::now().time_since_epoch()).count(); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=120" }, + Request* req2 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=120" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req2); 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_issue_1369.cpp b/test/storage/http_issue_1369.cpp index 44c190c8253..954cb3b3f56 100644 --- a/test/storage/http_issue_1369.cpp +++ b/test/storage/http_issue_1369.cpp @@ -30,7 +30,8 @@ TEST_F(Storage, HTTPIssue1369) { ADD_FAILURE() << "Callback should not be called"; }); fs.cancel(req); - fs.request(resource, uv_default_loop(), [&](const Response &res) { + req = fs.request(resource, uv_default_loop(), [&](const Response &res) { + fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(0, res.expires); diff --git a/test/storage/http_load.cpp b/test/storage/http_load.cpp index 2347a76ab4d..fa0468a8485 100644 --- a/test/storage/http_load.cpp +++ b/test/storage/http_load.cpp @@ -15,11 +15,15 @@ TEST_F(Storage, HTTPLoad) { const int max = 10000; int number = 1; - std::function req = [&]() { + Request* reqs[concurrency]; + + std::function req = [&](int i) { const auto current = number++; - fs.request({ Resource::Unknown, + reqs[i] = fs.request({ Resource::Unknown, std::string("http://127.0.0.1:3000/load/") + std::to_string(current) }, - uv_default_loop(), [&, current](const Response &res) { + uv_default_loop(), [&, i, current](const Response &res) { + fs.cancel(reqs[i]); + reqs[i] = nullptr; EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(std::string("Request ") + std::to_string(current), res.data); EXPECT_EQ(0, res.expires); @@ -28,7 +32,7 @@ TEST_F(Storage, HTTPLoad) { EXPECT_EQ("", res.message); if (number <= max) { - req(); + req(i); } else if (current == max) { HTTPLoad.finish(); } @@ -37,7 +41,7 @@ TEST_F(Storage, HTTPLoad) { for (int i = 0; i < concurrency; i++) { - req(); + req(i); } uv_run(uv_default_loop(), UV_RUN_DEFAULT); diff --git a/test/storage/http_other_loop.cpp b/test/storage/http_other_loop.cpp index 64612f13df2..43a655fba43 100644 --- a/test/storage/http_other_loop.cpp +++ b/test/storage/http_other_loop.cpp @@ -12,8 +12,9 @@ TEST_F(Storage, HTTPOtherLoop) { // This file source launches a separate thread to do the processing. DefaultFileSource fs(nullptr); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), + Request* req = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(0, res.expires); diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp index 78f69cae302..84af943025b 100644 --- a/test/storage/http_reading.cpp +++ b/test/storage/http_reading.cpp @@ -3,6 +3,7 @@ #include #include +#include #include @@ -17,8 +18,9 @@ TEST_F(Storage, HTTPReading) { const auto mainThread = uv_thread_self(); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), + Request* req1 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req1); EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); @@ -29,8 +31,9 @@ TEST_F(Storage, HTTPReading) { HTTPTest.finish(); }); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/doesnotexist" }, uv_default_loop(), + Request* req2 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/doesnotexist" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req2); EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::NotFound, res.status); EXPECT_EQ("", res.message); @@ -40,8 +43,9 @@ TEST_F(Storage, HTTPReading) { HTTP404.finish(); }); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/permanent-error" }, uv_default_loop(), + Request* req3 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/permanent-error" }, uv_default_loop(), [&](const Response &res) { + fs.cancel(req3); EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Error, res.status); EXPECT_EQ("HTTP status code 500", res.message); @@ -61,10 +65,14 @@ TEST_F(Storage, HTTPNoCallback) { DefaultFileSource fs(nullptr); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), + try { + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), nullptr); - - uv_run(uv_default_loop(), UV_RUN_DEFAULT); + } catch (const util::MisuseException& ex) { + EXPECT_EQ(std::string(ex.what()), "FileSource callback can't be empty"); + } catch (const std::exception&) { + EXPECT_TRUE(false) << "Unhandled exception."; + } HTTPTest.finish(); } From 096a3edf39d23fbd4baa134938c16fed4f2e199c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Thu, 15 Oct 2015 15:46:12 +0200 Subject: [PATCH 2/8] [core] use RAII-style lifetime tracking of Request objects --- src/mbgl/map/map_context.cpp | 15 +++------------ src/mbgl/map/map_context.hpp | 2 +- src/mbgl/map/raster_tile_data.cpp | 6 +----- src/mbgl/map/raster_tile_data.hpp | 3 ++- src/mbgl/map/source.cpp | 8 +------- src/mbgl/map/source.hpp | 3 ++- src/mbgl/map/sprite.cpp | 18 +++--------------- src/mbgl/map/vector_tile_data.cpp | 6 +----- src/mbgl/map/vector_tile_data.hpp | 3 ++- src/mbgl/storage/request_holder.cpp | 12 ++++++++++++ src/mbgl/storage/request_holder.hpp | 26 ++++++++++++++++++++++++++ src/mbgl/text/glyph_pbf.cpp | 8 +------- src/mbgl/text/glyph_pbf.hpp | 3 ++- 13 files changed, 57 insertions(+), 56 deletions(-) create mode 100644 src/mbgl/storage/request_holder.cpp create mode 100644 src/mbgl/storage/request_holder.hpp diff --git a/src/mbgl/map/map_context.cpp b/src/mbgl/map/map_context.cpp index 573882da87e..0888ef28460 100644 --- a/src/mbgl/map/map_context.cpp +++ b/src/mbgl/map/map_context.cpp @@ -52,10 +52,7 @@ MapContext::~MapContext() { void MapContext::cleanup() { view.notify(); - if (styleRequest) { - util::ThreadContext::getFileSource()->cancel(styleRequest); - styleRequest = nullptr; - } + styleRequest = nullptr; // Explicit resets currently necessary because these abandon resources that need to be // cleaned up by glObjectStore.performCleanup(); @@ -95,13 +92,7 @@ void MapContext::setStyleURL(const std::string& url) { return; } - FileSource* fs = util::ThreadContext::getFileSource(); - - if (styleRequest) { - fs->cancel(styleRequest); - styleRequest = nullptr; - } - + styleRequest = nullptr; styleURL = url; styleJSON.clear(); @@ -113,8 +104,8 @@ void MapContext::setStyleURL(const std::string& url) { base = styleURL.substr(0, pos + 1); } + FileSource* fs = util::ThreadContext::getFileSource(); styleRequest = fs->request({ Resource::Kind::Style, styleURL }, util::RunLoop::getLoop(), [this, base](const Response &res) { - util::ThreadContext::getFileSource()->cancel(styleRequest); styleRequest = nullptr; if (res.status == Response::Successful) { diff --git a/src/mbgl/map/map_context.hpp b/src/mbgl/map/map_context.hpp index b78d3283a76..028a05e4bb4 100644 --- a/src/mbgl/map/map_context.hpp +++ b/src/mbgl/map/map_context.hpp @@ -90,7 +90,7 @@ class MapContext : public Style::Observer { std::string styleURL; std::string styleJSON; - Request* styleRequest = nullptr; + RequestHolder styleRequest; Map::StillImageCallback callback; size_t sourceCacheSize; diff --git a/src/mbgl/map/raster_tile_data.cpp b/src/mbgl/map/raster_tile_data.cpp index c983a573752..4ae9189a0b6 100644 --- a/src/mbgl/map/raster_tile_data.cpp +++ b/src/mbgl/map/raster_tile_data.cpp @@ -31,7 +31,6 @@ void RasterTileData::request(float pixelRatio, FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Tile, url }, util::RunLoop::getLoop(), [url, callback, this](const Response &res) { - util::ThreadContext::getFileSource()->cancel(req); req = nullptr; if (res.status == Response::NotFound) { @@ -78,9 +77,6 @@ void RasterTileData::cancel() { if (state != State::obsolete) { state = State::obsolete; } - if (req) { - util::ThreadContext::getFileSource()->cancel(req); - req = nullptr; - } + req = nullptr; workRequest.reset(); } diff --git a/src/mbgl/map/raster_tile_data.hpp b/src/mbgl/map/raster_tile_data.hpp index f5711e506ad..5c8b42ef961 100644 --- a/src/mbgl/map/raster_tile_data.hpp +++ b/src/mbgl/map/raster_tile_data.hpp @@ -4,6 +4,7 @@ #include #include #include +#include namespace mbgl { @@ -28,7 +29,7 @@ class RasterTileData : public TileData { private: const SourceInfo& source; Worker& worker; - Request* req = nullptr; + RequestHolder req; RasterLayoutProperties layout; RasterBucket bucket; diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index 3e8aba8859b..d723e308b19 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -122,12 +122,7 @@ std::string SourceInfo::tileURL(const TileID& id, float pixelRatio) const { Source::Source() {} -Source::~Source() { - if (req) { - util::ThreadContext::getFileSource()->cancel(req); - req = nullptr; - } -} +Source::~Source() = default; bool Source::isLoaded() const { if (!loaded) { @@ -154,7 +149,6 @@ void Source::load() { FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Source, info.url }, util::RunLoop::getLoop(), [this](const Response &res) { - util::ThreadContext::getFileSource()->cancel(req); req = nullptr; if (res.status != Response::Successful) { diff --git a/src/mbgl/map/source.hpp b/src/mbgl/map/source.hpp index fceb5962a4f..f5b95ef8741 100644 --- a/src/mbgl/map/source.hpp +++ b/src/mbgl/map/source.hpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -129,7 +130,7 @@ class Source : private util::noncopyable { std::map> tile_data; TileCache cache; - Request* req = nullptr; + RequestHolder req; Observer* observer_ = nullptr; }; diff --git a/src/mbgl/map/sprite.cpp b/src/mbgl/map/sprite.cpp index 02bf0895c14..9c099b4aa2d 100644 --- a/src/mbgl/map/sprite.cpp +++ b/src/mbgl/map/sprite.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -23,19 +24,8 @@ struct Sprite::Loader { bool loadedImage = false; std::unique_ptr data = std::make_unique(); - Request* jsonRequest = nullptr; - Request* spriteRequest = nullptr; - - ~Loader() { - if (jsonRequest) { - util::ThreadContext::getFileSource()->cancel(jsonRequest); - jsonRequest = nullptr; - } - if (spriteRequest) { - util::ThreadContext::getFileSource()->cancel(spriteRequest); - spriteRequest = nullptr; - } - } + RequestHolder jsonRequest; + RequestHolder spriteRequest; }; Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) @@ -54,7 +44,6 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) FileSource* fs = util::ThreadContext::getFileSource(); loader->jsonRequest = fs->request({ Resource::Kind::SpriteJSON, jsonURL }, util::RunLoop::getLoop(), [this, jsonURL](const Response& res) { - util::ThreadContext::getFileSource()->cancel(loader->jsonRequest); loader->jsonRequest = nullptr; if (res.status == Response::Successful) { loader->data->json = res.data; @@ -71,7 +60,6 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) loader->spriteRequest = fs->request({ Resource::Kind::SpriteImage, spriteURL }, util::RunLoop::getLoop(), [this, spriteURL](const Response& res) { - util::ThreadContext::getFileSource()->cancel(loader->spriteRequest); loader->spriteRequest = nullptr; if (res.status == Response::Successful) { loader->data->image = res.data; diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp index a10dd64bd53..6aa643b16f5 100644 --- a/src/mbgl/map/vector_tile_data.cpp +++ b/src/mbgl/map/vector_tile_data.cpp @@ -43,7 +43,6 @@ void VectorTileData::request(float pixelRatio, const std::function& call FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Tile, url }, util::RunLoop::getLoop(), [url, callback, this](const Response &res) { - util::ThreadContext::getFileSource()->cancel(req); req = nullptr; if (res.status == Response::NotFound) { @@ -139,9 +138,6 @@ void VectorTileData::cancel() { if (state != State::obsolete) { state = State::obsolete; } - if (req) { - util::ThreadContext::getFileSource()->cancel(req); - req = nullptr; - } + req = nullptr; workRequest.reset(); } diff --git a/src/mbgl/map/vector_tile_data.hpp b/src/mbgl/map/vector_tile_data.hpp index c004e804b7a..1a1ff84061a 100644 --- a/src/mbgl/map/vector_tile_data.hpp +++ b/src/mbgl/map/vector_tile_data.hpp @@ -3,6 +3,7 @@ #include #include +#include #include @@ -40,7 +41,7 @@ class VectorTileData : public TileData { std::unique_ptr workRequest; bool parsing = false; const SourceInfo& source; - Request* req = nullptr; + RequestHolder req; std::string data; float lastAngle = 0; float currentAngle; diff --git a/src/mbgl/storage/request_holder.cpp b/src/mbgl/storage/request_holder.cpp new file mode 100644 index 00000000000..3a038623c47 --- /dev/null +++ b/src/mbgl/storage/request_holder.cpp @@ -0,0 +1,12 @@ +#include +#include +#include + +namespace mbgl { + +void RequestHolder::Deleter::operator()(Request* req) const { + // This function is called by the unique_ptr's Deleter. + util::ThreadContext::getFileSource()->cancel(req); +} + +} diff --git a/src/mbgl/storage/request_holder.hpp b/src/mbgl/storage/request_holder.hpp new file mode 100644 index 00000000000..62edbfde7d0 --- /dev/null +++ b/src/mbgl/storage/request_holder.hpp @@ -0,0 +1,26 @@ +#ifndef MBGL_STORAGE_REQUEST_HOLDER +#define MBGL_STORAGE_REQUEST_HOLDER + +#include + +namespace mbgl { + +class Request; + +class RequestHolder { +public: + inline RequestHolder& operator=(Request* req) { + ptr = std::unique_ptr(req); + return *this; + } + +private: + struct Deleter { + void operator()(Request*) const; + }; + std::unique_ptr ptr; +}; + +} + +#endif diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp index f9f8afcb1bd..e37e656d91a 100644 --- a/src/mbgl/text/glyph_pbf.cpp +++ b/src/mbgl/text/glyph_pbf.cpp @@ -75,7 +75,6 @@ GlyphPBF::GlyphPBF(GlyphStore* store, }); auto requestCallback = [this, store, fontStack, url](const Response &res) { - util::ThreadContext::getFileSource()->cancel(req); req = nullptr; if (res.status != Response::Successful) { @@ -92,12 +91,7 @@ GlyphPBF::GlyphPBF(GlyphStore* store, req = fs->request({ Resource::Kind::Glyphs, url }, util::RunLoop::getLoop(), requestCallback); } -GlyphPBF::~GlyphPBF() { - if (req) { - util::ThreadContext::getFileSource()->cancel(req); - req = nullptr; - } -} +GlyphPBF::~GlyphPBF() = default; void GlyphPBF::parse(GlyphStore* store, const std::string& fontStack, const std::string& url) { if (data.empty()) { diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp index 2aa2134d168..205824bfe5b 100644 --- a/src/mbgl/text/glyph_pbf.hpp +++ b/src/mbgl/text/glyph_pbf.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -44,7 +45,7 @@ class GlyphPBF : private util::noncopyable { std::string data; std::atomic parsed; - Request* req = nullptr; + RequestHolder req; Observer* observer = nullptr; }; From 6a7334b882a47ca193209f2012843e42aa3ed4e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Thu, 15 Oct 2015 21:30:21 +0200 Subject: [PATCH 3/8] [core] add support for stale responses We're now returning stale responses from cache. Those responses will have the `stale` flag set to true. Currently, all requesters in the core code discard stale responses, and cancel the request immediately after they got a non-stale response. --- include/mbgl/storage/response.hpp | 4 + src/mbgl/map/map_context.cpp | 4 + src/mbgl/map/raster_tile_data.cpp | 4 + src/mbgl/map/source.cpp | 4 + src/mbgl/map/sprite.cpp | 8 + src/mbgl/map/vector_tile_data.cpp | 4 + src/mbgl/storage/default_file_source.cpp | 78 +++++++--- src/mbgl/storage/default_file_source_impl.hpp | 2 + src/mbgl/storage/request.cpp | 1 - src/mbgl/storage/response.cpp | 12 ++ src/mbgl/text/glyph_pbf.cpp | 4 + test/storage/cache_response.cpp | 30 ++-- test/storage/cache_revalidate.cpp | 139 ++++++++++++++++-- test/storage/directory_reading.cpp | 1 + test/storage/file_reading.cpp | 3 + test/storage/http_cancel.cpp | 1 + test/storage/http_coalescing.cpp | 105 +++++++++++++ test/storage/http_error.cpp | 2 + test/storage/http_header_parsing.cpp | 2 + test/storage/http_issue_1369.cpp | 1 + test/storage/http_load.cpp | 1 + test/storage/http_other_loop.cpp | 1 + test/storage/http_reading.cpp | 3 + 23 files changed, 367 insertions(+), 47 deletions(-) create mode 100644 src/mbgl/storage/response.cpp diff --git a/include/mbgl/storage/response.hpp b/include/mbgl/storage/response.hpp index e665f177fca..b232cd06f4e 100644 --- a/include/mbgl/storage/response.hpp +++ b/include/mbgl/storage/response.hpp @@ -6,10 +6,14 @@ namespace mbgl { class Response { +public: + bool isExpired() const; + public: enum Status { Error, Successful, NotFound }; Status status = Error; + bool stale = false; std::string message; int64_t modified = 0; int64_t expires = 0; diff --git a/src/mbgl/map/map_context.cpp b/src/mbgl/map/map_context.cpp index 0888ef28460..b197ea12f3e 100644 --- a/src/mbgl/map/map_context.cpp +++ b/src/mbgl/map/map_context.cpp @@ -106,6 +106,10 @@ void MapContext::setStyleURL(const std::string& url) { FileSource* fs = util::ThreadContext::getFileSource(); styleRequest = fs->request({ Resource::Kind::Style, styleURL }, util::RunLoop::getLoop(), [this, base](const Response &res) { + if (res.stale) { + // Only handle fresh responses. + return; + } styleRequest = nullptr; if (res.status == Response::Successful) { diff --git a/src/mbgl/map/raster_tile_data.cpp b/src/mbgl/map/raster_tile_data.cpp index 4ae9189a0b6..cc7b6b548fe 100644 --- a/src/mbgl/map/raster_tile_data.cpp +++ b/src/mbgl/map/raster_tile_data.cpp @@ -31,6 +31,10 @@ void RasterTileData::request(float pixelRatio, FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Tile, url }, util::RunLoop::getLoop(), [url, callback, this](const Response &res) { + if (res.stale) { + // Only handle fresh responses. + return; + } req = nullptr; if (res.status == Response::NotFound) { diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index d723e308b19..7e306828950 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -149,6 +149,10 @@ void Source::load() { FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Source, info.url }, util::RunLoop::getLoop(), [this](const Response &res) { + if (res.stale) { + // Only handle fresh responses. + return; + } req = nullptr; if (res.status != Response::Successful) { diff --git a/src/mbgl/map/sprite.cpp b/src/mbgl/map/sprite.cpp index 9c099b4aa2d..a54d96f005c 100644 --- a/src/mbgl/map/sprite.cpp +++ b/src/mbgl/map/sprite.cpp @@ -44,6 +44,10 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) FileSource* fs = util::ThreadContext::getFileSource(); loader->jsonRequest = fs->request({ Resource::Kind::SpriteJSON, jsonURL }, util::RunLoop::getLoop(), [this, jsonURL](const Response& res) { + if (res.stale) { + // Only handle fresh responses. + return; + } loader->jsonRequest = nullptr; if (res.status == Response::Successful) { loader->data->json = res.data; @@ -60,6 +64,10 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) loader->spriteRequest = fs->request({ Resource::Kind::SpriteImage, spriteURL }, util::RunLoop::getLoop(), [this, spriteURL](const Response& res) { + if (res.stale) { + // Only handle fresh responses. + return; + } loader->spriteRequest = nullptr; if (res.status == Response::Successful) { loader->data->image = res.data; diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp index 6aa643b16f5..2e683daaffe 100644 --- a/src/mbgl/map/vector_tile_data.cpp +++ b/src/mbgl/map/vector_tile_data.cpp @@ -43,6 +43,10 @@ void VectorTileData::request(float pixelRatio, const std::function& call FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Tile, url }, util::RunLoop::getLoop(), [url, callback, this](const Response &res) { + if (res.stale) { + // Only handle fresh responses. + return; + } req = nullptr; if (res.status == Response::NotFound) { diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 7b3cff82533..71c42f6220d 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -8,7 +8,6 @@ #include #include -#include #include #include #include @@ -102,43 +101,79 @@ void DefaultFileSource::Impl::add(Request* req) { const Resource& resource = req->resource; DefaultFileRequest* request = find(resource); - if (request) { - request->observers.insert(req); - return; + if (!request) { + request = &pending.emplace(resource, resource).first->second; } - request = &pending.emplace(resource, resource).first->second; + // Add this request as an observer so that it'll get notified when something about this + // request changes. request->observers.insert(req); - if (cache) { - startCacheRequest(request); + update(request); + + if (request->response) { + // We've got a response, so send the (potentially stale) response to the requester. + req->notify(request->response); + } +} + +void DefaultFileSource::Impl::update(DefaultFileRequest* request) { + if (request->response) { + // We've at least obtained a cache value, potentially we also got a final response. + // The observers have been notified already; send what we have to the new one as well. + + // Before returning the existing response, make sure that it is still fresh. + if (!request->response->stale && request->response->isExpired()) { + // Create a new Response object with `stale = true`, but the same data, and + // replace the current request object we have. + // TODO: Make content shared_ptrs so we won't make copies of the content. + auto response = std::make_shared(*request->response); + response->stale = true; + request->response = response; + } + + if (request->response->stale && !request->realRequest) { + // We've returned a stale response; now make sure the requester also gets a fresh + // response eventually. It's possible that there's already a request in progress. + // Note that this will also trigger updates to all other existing listeners. + // Since we already have data, we're going to verify + startRealRequest(request, request->response); + } + } else if (!request->cacheRequest && !request->realRequest) { + // There is no request in progress, and we don't have a response yet. This means we'll have + // to start the request ourselves. + if (cache) { + startCacheRequest(request); + } else { + startRealRequest(request); + } } else { - startRealRequest(request); + // There is a request in progress. We just have to wait. } } void DefaultFileSource::Impl::startCacheRequest(DefaultFileRequest* request) { // Check the cache for existing data so that we can potentially // revalidate the information without having to redownload everything. - request->cacheRequest = cache->get(request->resource, [this, request](std::unique_ptr response) { - auto expired = [&response] { - const int64_t now = std::chrono::duration_cast( - SystemClock::now().time_since_epoch()).count(); - return response->expires <= now; - }; - - if (!response || expired()) { + request->cacheRequest = cache->get(request->resource, [this, request](std::shared_ptr response) { + request->cacheRequest = nullptr; + if (response) { + response->stale = response->isExpired(); + + // Notify in all cases; requestors can decide whether they want to use stale responses. + notify(request, response, FileCache::Hint::No); + } + + if (!response || response->stale) { // No response or stale cache. Run the real request. - startRealRequest(request, std::move(response)); - } else { - // The response is fresh. We're good to notify the caller. - notify(request, std::move(response), FileCache::Hint::No); + startRealRequest(request, response); } }); } void DefaultFileSource::Impl::startRealRequest(DefaultFileRequest* request, std::shared_ptr response) { auto callback = [request, this] (std::shared_ptr res, FileCache::Hint hint) { + request->realRequest = nullptr; notify(request, res, hint); }; @@ -181,6 +216,7 @@ void DefaultFileSource::Impl::notify(DefaultFileRequest* request, std::shared_pt assert(response); // Notify all observers. + request->response = response; for (auto req : request->observers) { req->notify(response); } @@ -189,8 +225,6 @@ void DefaultFileSource::Impl::notify(DefaultFileRequest* request, std::shared_pt // Store response in database 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 bb289337983..980eedc1180 100644 --- a/src/mbgl/storage/default_file_source_impl.hpp +++ b/src/mbgl/storage/default_file_source_impl.hpp @@ -15,6 +15,7 @@ class RequestBase; struct DefaultFileRequest { const Resource resource; std::set observers; + std::shared_ptr response; std::unique_ptr cacheRequest; RequestBase* realRequest = nullptr; @@ -39,6 +40,7 @@ class DefaultFileSource::Impl { private: DefaultFileRequest* find(const Resource&); + void update(DefaultFileRequest*); void startCacheRequest(DefaultFileRequest*); void startRealRequest(DefaultFileRequest*, std::shared_ptr = nullptr); void notify(DefaultFileRequest*, std::shared_ptr, FileCache::Hint); diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index f8ad555379a..55913846cf9 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -49,7 +49,6 @@ Request::~Request() = default; // Called in the FileSource thread. void Request::notify(const std::shared_ptr &response_) { - assert(!std::atomic_load(&response)); assert(response_); std::atomic_store(&response, response_); async->send(); diff --git a/src/mbgl/storage/response.cpp b/src/mbgl/storage/response.cpp new file mode 100644 index 00000000000..628a2a3b99a --- /dev/null +++ b/src/mbgl/storage/response.cpp @@ -0,0 +1,12 @@ +#include +#include + +namespace mbgl { + +bool Response::isExpired() const { + const int64_t now = std::chrono::duration_cast( + SystemClock::now().time_since_epoch()).count(); + return expires <= now; +} + +} // namespace mbgl diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp index e37e656d91a..f351e66c2a0 100644 --- a/src/mbgl/text/glyph_pbf.cpp +++ b/src/mbgl/text/glyph_pbf.cpp @@ -75,6 +75,10 @@ GlyphPBF::GlyphPBF(GlyphStore* store, }); auto requestCallback = [this, store, fontStack, url](const Response &res) { + if (res.stale) { + // Only handle fresh responses. + return; + } req = nullptr; if (res.status != Response::Successful) { diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp index 0fba2ba5e77..12945138587 100644 --- a/test/storage/cache_response.cpp +++ b/test/storage/cache_response.cpp @@ -14,27 +14,35 @@ TEST_F(Storage, CacheResponse) { DefaultFileSource fs(&cache); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/cache" }; + Response response; Request* req = fs.request(resource, uv_default_loop(), [&](const Response &res) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Response 1", res.data); EXPECT_LT(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); EXPECT_EQ("", res.message); + response = res; + }); + + uv_run(uv_default_loop(), UV_RUN_DEFAULT); - req = fs.request(resource, uv_default_loop(), [&, res](const Response &res2) { - fs.cancel(req); - EXPECT_EQ(res.status, res2.status); - EXPECT_EQ(res.data, res2.data); - EXPECT_EQ(res.expires, res2.expires); - EXPECT_EQ(res.modified, res2.modified); - EXPECT_EQ(res.etag, res2.etag); - EXPECT_EQ(res.message, res2.message); - - CacheResponse.finish(); - }); + // Now test that we get the same values as in the previous request. If we'd go to the server + // again, we'd get different values. + req = fs.request(resource, uv_default_loop(), [&](const Response &res) { + fs.cancel(req); + EXPECT_EQ(response.status, res.status); + EXPECT_EQ(response.stale, res.stale); + EXPECT_EQ(response.data, res.data); + EXPECT_EQ(response.expires, res.expires); + EXPECT_EQ(response.modified, res.modified); + EXPECT_EQ(response.etag, res.etag); + EXPECT_EQ(response.message, res.message); + + CacheResponse.finish(); }); uv_run(uv_default_loop(), UV_RUN_DEFAULT); diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp index 8ebedef01b7..90633b83ac5 100644 --- a/test/storage/cache_revalidate.cpp +++ b/test/storage/cache_revalidate.cpp @@ -5,29 +5,58 @@ #include #include -TEST_F(Storage, CacheRevalidate) { +TEST_F(Storage, CacheRevalidateSame) { SCOPED_TEST(CacheRevalidateSame) - SCOPED_TEST(CacheRevalidateModified) - SCOPED_TEST(CacheRevalidateEtag) using namespace mbgl; SQLiteCache cache(":memory:"); DefaultFileSource fs(&cache); + const Response *reference = nullptr; + const Resource revalidateSame { Resource::Unknown, "http://127.0.0.1:3000/revalidate-same" }; - Request* req = fs.request(revalidateSame, uv_default_loop(), [&](const Response &res) { - fs.cancel(req); + Request* req1 = nullptr; + Request* req2 = nullptr; + req1 = fs.request(revalidateSame, uv_default_loop(), [&](const Response &res) { + // This callback can get triggered multiple times. We only care about the first invocation. + // It will get triggered again when refreshing the req2 (see below). + static bool first = true; + if (!first) { + return; + } + first = false; + + EXPECT_EQ(nullptr, reference); + reference = &res; + EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Response", res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("snowfall", res.etag); EXPECT_EQ("", res.message); - req = fs.request(revalidateSame, uv_default_loop(), [&, res](const Response &res2) { - fs.cancel(req); + req2 = fs.request(revalidateSame, uv_default_loop(), [&, res](const Response &res2) { + // Make sure we get a different object than before, since this request should've been revalidated. + EXPECT_TRUE(reference != &res2); + + if (res2.stale) { + // Discard stale responses, if any. + return; + } + + ASSERT_TRUE(req1); + fs.cancel(req1); + req1 = nullptr; + + ASSERT_TRUE(req2); + fs.cancel(req2); + req2 = nullptr; + EXPECT_EQ(Response::Successful, res2.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Response", res2.data); // We use this to indicate that a 304 reply came back. EXPECT_LT(0, res2.expires); @@ -40,11 +69,37 @@ TEST_F(Storage, CacheRevalidate) { }); }); + uv_run(uv_default_loop(), UV_RUN_DEFAULT); +} + +TEST_F(Storage, CacheRevalidateModified) { + SCOPED_TEST(CacheRevalidateModified) + + using namespace mbgl; + + SQLiteCache cache(":memory:"); + DefaultFileSource fs(&cache); + + const Response *reference = nullptr; + const Resource revalidateModified{ Resource::Unknown, "http://127.0.0.1:3000/revalidate-modified" }; - Request* req2 = fs.request(revalidateModified, uv_default_loop(), [&](const Response &res) { - fs.cancel(req2); + Request* req1 = nullptr; + Request* req2 = nullptr; + req1 = fs.request(revalidateModified, uv_default_loop(), [&](const Response& res) { + // This callback can get triggered multiple times. We only care about the first invocation. + // It will get triggered again when refreshing the req2 (see below). + static bool first = true; + if (!first) { + return; + } + first = false; + + EXPECT_EQ(nullptr, reference); + reference = &res; + EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Response", res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(1420070400, res.modified); @@ -52,8 +107,24 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("", res.message); req2 = fs.request(revalidateModified, uv_default_loop(), [&, res](const Response &res2) { + // Make sure we get a different object than before, since this request should've been revalidated. + EXPECT_TRUE(reference != &res2); + + if (res2.stale) { + // Discard stale responses, if any. + return; + } + + ASSERT_TRUE(req1); + fs.cancel(req1); + req1 = nullptr; + + ASSERT_TRUE(req2); fs.cancel(req2); + req2 = nullptr; + EXPECT_EQ(Response::Successful, res2.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Response", res2.data); // We use this to indicate that a 304 reply came back. EXPECT_LT(0, res2.expires); @@ -65,19 +136,61 @@ TEST_F(Storage, CacheRevalidate) { }); }); + uv_run(uv_default_loop(), UV_RUN_DEFAULT); +} + +TEST_F(Storage, CacheRevalidateEtag) { + SCOPED_TEST(CacheRevalidateEtag) + + using namespace mbgl; + + SQLiteCache cache(":memory:"); + DefaultFileSource fs(&cache); + + const Response *reference = nullptr; + const Resource revalidateEtag { Resource::Unknown, "http://127.0.0.1:3000/revalidate-etag" }; - Request* req3 = fs.request(revalidateEtag, uv_default_loop(), [&](const Response &res) { - fs.cancel(req3); + Request* req1 = nullptr; + Request* req2 = nullptr; + req1 = fs.request(revalidateEtag, uv_default_loop(), [&](const Response &res) { + // This callback can get triggered multiple times. We only care about the first invocation. + // It will get triggered again when refreshing the req2 (see below). + static bool first = true; + if (!first) { + return; + } + first = false; + + EXPECT_EQ(nullptr, reference); + reference = &res; + EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Response 1", res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("response-1", res.etag); EXPECT_EQ("", res.message); - req3 = fs.request(revalidateEtag, uv_default_loop(), [&, res](const Response &res2) { - fs.cancel(req3); + req2 = fs.request(revalidateEtag, uv_default_loop(), [&, res](const Response &res2) { + // Make sure we get a different object than before, since this request should've been revalidated. + EXPECT_TRUE(reference != &res2); + + if (res2.stale) { + // Discard stale responses, if any. + return; + } + + ASSERT_TRUE(req1); + fs.cancel(req1); + req1 = nullptr; + + ASSERT_TRUE(req2); + fs.cancel(req2); + req2 = nullptr; + EXPECT_EQ(Response::Successful, res2.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Response 2", res2.data); EXPECT_EQ(0, res2.expires); EXPECT_EQ(0, res2.modified); diff --git a/test/storage/directory_reading.cpp b/test/storage/directory_reading.cpp index 11298dff8b8..f0bc7ea6d2b 100644 --- a/test/storage/directory_reading.cpp +++ b/test/storage/directory_reading.cpp @@ -19,6 +19,7 @@ TEST_F(Storage, AssetReadDirectory) { [&](const Response &res) { fs.cancel(req); EXPECT_EQ(Response::Error, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ(0ul, res.data.size()); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); diff --git a/test/storage/file_reading.cpp b/test/storage/file_reading.cpp index dc0620b78b6..434af703f2d 100644 --- a/test/storage/file_reading.cpp +++ b/test/storage/file_reading.cpp @@ -20,6 +20,7 @@ TEST_F(Storage, AssetEmptyFile) { [&](const Response &res) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ(0ul, res.data.size()); EXPECT_EQ(0, res.expires); EXPECT_LT(1420000000, res.modified); @@ -46,6 +47,7 @@ TEST_F(Storage, AssetNonEmptyFile) { uv_default_loop(), [&](const Response &res) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ(16ul, res.data.size()); EXPECT_EQ(0, res.expires); EXPECT_LT(1420000000, res.modified); @@ -73,6 +75,7 @@ TEST_F(Storage, AssetNonExistentFile) { uv_default_loop(), [&](const Response &res) { fs.cancel(req); EXPECT_EQ(Response::Error, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ(0ul, res.data.size()); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); diff --git a/test/storage/http_cancel.cpp b/test/storage/http_cancel.cpp index 2eb599eeaa5..5743ccb8f77 100644 --- a/test/storage/http_cancel.cpp +++ b/test/storage/http_cancel.cpp @@ -39,6 +39,7 @@ TEST_F(Storage, HTTPCancelMultiple) { Request* req = fs.request(resource, uv_default_loop(), [&](const Response &res) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); diff --git a/test/storage/http_coalescing.cpp b/test/storage/http_coalescing.cpp index 9dfa855501b..85583620343 100644 --- a/test/storage/http_coalescing.cpp +++ b/test/storage/http_coalescing.cpp @@ -50,3 +50,108 @@ TEST_F(Storage, HTTPCoalescing) { uv_run(uv_default_loop(), UV_RUN_DEFAULT); } + +TEST_F(Storage, HTTPMultiple) { + SCOPED_TEST(HTTPMultiple) + + using namespace mbgl; + + DefaultFileSource fs(nullptr); + + const Response *reference = nullptr; + + const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test?expires=2147483647" }; + Request* req1 = nullptr; + Request* req2 = nullptr; + req1 = fs.request(resource, uv_default_loop(), [&] (const Response &res) { + EXPECT_EQ(nullptr, reference); + reference = &res; + + // Do not cancel the request right away. + EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ("Hello World!", res.data); + EXPECT_EQ(2147483647, res.expires); + EXPECT_EQ(0, res.modified); + EXPECT_EQ("", res.etag); + EXPECT_EQ("", res.message); + + // Start a second request for the same resource after the first one has been completed. + req2 = fs.request(resource, uv_default_loop(), [&] (const Response &res2) { + // Make sure we get the same object ID as before. + EXPECT_EQ(reference, &res2); + + // Now cancel both requests after both have been notified. + fs.cancel(req1); + fs.cancel(req2); + + EXPECT_EQ(Response::Successful, res2.status); + EXPECT_EQ("Hello World!", res2.data); + EXPECT_EQ(2147483647, res2.expires); + EXPECT_EQ(0, res2.modified); + EXPECT_EQ("", res2.etag); + EXPECT_EQ("", res2.message); + + HTTPMultiple.finish(); + }); + }); + + uv_run(uv_default_loop(), UV_RUN_DEFAULT); +} + +// Tests that we get stale responses from previous requests when requesting the same thing again. +TEST_F(Storage, HTTPStale) { + SCOPED_TEST(HTTPStale) + + using namespace mbgl; + + DefaultFileSource fs(nullptr); + + int updates = 0; + int stale = 0; + + const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; + Request* req1 = nullptr; + Request* req2 = nullptr; + req1 = fs.request(resource, uv_default_loop(), [&] (const Response &res) { + // Do not cancel the request right away. + EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ("Hello World!", res.data); + EXPECT_EQ(false, res.stale); + EXPECT_EQ(0, res.expires); + EXPECT_EQ(0, res.modified); + EXPECT_EQ("", res.etag); + EXPECT_EQ("", res.message); + + // Don't start the request twice in case this callback gets fired multiple times. + if (req2) { + return; + } + + updates++; + + // Start a second request for the same resource after the first one has been completed. + req2 = fs.request(resource, uv_default_loop(), [&] (const Response &res2) { + EXPECT_EQ(Response::Successful, res2.status); + EXPECT_EQ("Hello World!", res2.data); + EXPECT_EQ(0, res2.expires); + EXPECT_EQ(0, res2.modified); + EXPECT_EQ("", res2.etag); + EXPECT_EQ("", res2.message); + + if (res2.stale) { + EXPECT_EQ(0, stale); + stale++; + } else { + // Now cancel both requests after both have been notified. + fs.cancel(req1); + fs.cancel(req2); + HTTPStale.finish(); + } + }); + }); + + uv_run(uv_default_loop(), UV_RUN_DEFAULT); + + EXPECT_EQ(1, stale); + EXPECT_EQ(1, updates); +} diff --git a/test/storage/http_error.cpp b/test/storage/http_error.cpp index aebc75405d5..50b46a41b8b 100644 --- a/test/storage/http_error.cpp +++ b/test/storage/http_error.cpp @@ -37,6 +37,7 @@ TEST_F(Storage, HTTPError) { EXPECT_LT(1, duration) << "Backoff timer didn't wait 1 second"; EXPECT_GT(1.2, duration) << "Backoff timer fired too late"; EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); @@ -54,6 +55,7 @@ TEST_F(Storage, HTTPError) { EXPECT_LT(1.5, duration) << "Resource wasn't retried the correct number of times"; EXPECT_GT(1.7, duration) << "Resource wasn't retried the correct number of times"; EXPECT_EQ(Response::Error, res.status); + EXPECT_EQ(false, res.stale); #ifdef MBGL_HTTP_NSURL EXPECT_TRUE(res.message == "The operation couldn’t be completed. (NSURLErrorDomain error -1004.)" || diff --git a/test/storage/http_header_parsing.cpp b/test/storage/http_header_parsing.cpp index 13eab991255..93fdcb62313 100644 --- a/test/storage/http_header_parsing.cpp +++ b/test/storage/http_header_parsing.cpp @@ -20,6 +20,7 @@ TEST_F(Storage, HTTPHeaderParsing) { uv_default_loop(), [&](const Response &res) { fs.cancel(req1); EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(1420797926, res.expires); EXPECT_EQ(1420794326, res.modified); @@ -35,6 +36,7 @@ TEST_F(Storage, HTTPHeaderParsing) { uv_default_loop(), [&](const Response &res) { fs.cancel(req2); EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); 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"; EXPECT_EQ(0, res.modified); diff --git a/test/storage/http_issue_1369.cpp b/test/storage/http_issue_1369.cpp index 954cb3b3f56..ba4e3d38851 100644 --- a/test/storage/http_issue_1369.cpp +++ b/test/storage/http_issue_1369.cpp @@ -33,6 +33,7 @@ TEST_F(Storage, HTTPIssue1369) { req = fs.request(resource, uv_default_loop(), [&](const Response &res) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); diff --git a/test/storage/http_load.cpp b/test/storage/http_load.cpp index fa0468a8485..092bf2db5a5 100644 --- a/test/storage/http_load.cpp +++ b/test/storage/http_load.cpp @@ -25,6 +25,7 @@ TEST_F(Storage, HTTPLoad) { fs.cancel(reqs[i]); reqs[i] = nullptr; EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ(std::string("Request ") + std::to_string(current), res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); diff --git a/test/storage/http_other_loop.cpp b/test/storage/http_other_loop.cpp index 43a655fba43..cd9fad95be9 100644 --- a/test/storage/http_other_loop.cpp +++ b/test/storage/http_other_loop.cpp @@ -16,6 +16,7 @@ TEST_F(Storage, HTTPOtherLoop) { [&](const Response &res) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp index 84af943025b..c5e89b88e88 100644 --- a/test/storage/http_reading.cpp +++ b/test/storage/http_reading.cpp @@ -23,6 +23,7 @@ TEST_F(Storage, HTTPReading) { fs.cancel(req1); EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); @@ -36,6 +37,7 @@ TEST_F(Storage, HTTPReading) { fs.cancel(req2); EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::NotFound, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("", res.message); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); @@ -48,6 +50,7 @@ TEST_F(Storage, HTTPReading) { fs.cancel(req3); EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Error, res.status); + EXPECT_EQ(false, res.stale); EXPECT_EQ("HTTP status code 500", res.message); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); From 9bbd9aba3a8cebf7191ae5b28c8cf16acf39987e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Thu, 15 Oct 2015 23:02:11 +0200 Subject: [PATCH 4/8] [core] set a timer and auto-refresh requests when they expire When we get a request with an explicit expiration time, we're now starting a timer, and will trigger a refresh once the data expires. This gives requesters a chance to update their data. --- src/mbgl/storage/default_file_source.cpp | 35 ++++++++++++++++-- src/mbgl/storage/default_file_source_impl.hpp | 1 + test/storage/http_timeout.cpp | 37 +++++++++++++++++++ test/test.gypi | 1 + 4 files changed, 71 insertions(+), 3 deletions(-) create mode 100644 test/storage/http_timeout.cpp diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 71c42f6220d..5d698fc0f0d 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wshadow" @@ -159,19 +160,26 @@ void DefaultFileSource::Impl::startCacheRequest(DefaultFileRequest* request) { request->cacheRequest = nullptr; if (response) { response->stale = response->isExpired(); - - // Notify in all cases; requestors can decide whether they want to use stale responses. - notify(request, response, FileCache::Hint::No); } if (!response || response->stale) { // No response or stale cache. Run the real request. startRealRequest(request, response); } + + if (response) { + // Notify in all cases; requestors can decide whether they want to use stale responses. + notify(request, response, FileCache::Hint::No); + } }); } void DefaultFileSource::Impl::startRealRequest(DefaultFileRequest* request, std::shared_ptr response) { + // Cancel the timer if we have one. + if (request->timerRequest) { + request->timerRequest->stop(); + } + auto callback = [request, this] (std::shared_ptr res, FileCache::Hint hint) { request->realRequest = nullptr; notify(request, res, hint); @@ -225,6 +233,27 @@ void DefaultFileSource::Impl::notify(DefaultFileRequest* request, std::shared_pt // Store response in database cache->put(request->resource, response, hint); } + + // Set timer for requests that have a known expiry times. Expiry times of 0 are technically + // expiring immediately, but we can't continually request. + if (!request->realRequest && response->expires > 0) { + const int64_t now = std::chrono::duration_cast( + SystemClock::now().time_since_epoch()).count(); + const int64_t timeout = response->expires - now; + + if (timeout <= 0) { + update(request); + } else { + if (!request->timerRequest) { + request->timerRequest = std::make_unique(util::RunLoop::getLoop()); + } + + // timeout is in seconds, but the timer takes milliseconds. + request->timerRequest->start(1000 * timeout, 0, [this, request] { + update(request); + }); + } + } } } diff --git a/src/mbgl/storage/default_file_source_impl.hpp b/src/mbgl/storage/default_file_source_impl.hpp index 980eedc1180..387c6ce66ea 100644 --- a/src/mbgl/storage/default_file_source_impl.hpp +++ b/src/mbgl/storage/default_file_source_impl.hpp @@ -19,6 +19,7 @@ struct DefaultFileRequest { std::unique_ptr cacheRequest; RequestBase* realRequest = nullptr; + std::unique_ptr timerRequest; inline DefaultFileRequest(const Resource& resource_) : resource(resource_) {} diff --git a/test/storage/http_timeout.cpp b/test/storage/http_timeout.cpp new file mode 100644 index 00000000000..71553677bd2 --- /dev/null +++ b/test/storage/http_timeout.cpp @@ -0,0 +1,37 @@ +#include "storage.hpp" + +#include + +#include +#include + + +TEST_F(Storage, HTTPTimeout) { + SCOPED_TEST(HTTPTimeout) + + using namespace mbgl; + + DefaultFileSource fs(nullptr); + + int counter = 0; + + const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=1" }; + Request* req = fs.request(resource, uv_default_loop(), [&](const Response &res) { + counter++; + EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ(false, res.stale); + EXPECT_EQ("Hello World!", res.data); + EXPECT_LT(0, res.expires); + EXPECT_EQ(0, res.modified); + EXPECT_EQ("", res.etag); + EXPECT_EQ("", res.message); + if (counter == 4) { + fs.cancel(req); + HTTPTimeout.finish(); + } + }); + + uv_run(uv_default_loop(), UV_RUN_DEFAULT); + + EXPECT_EQ(4, counter); +} diff --git a/test/test.gypi b/test/test.gypi index b581053d81f..3631c14ef15 100644 --- a/test/test.gypi +++ b/test/test.gypi @@ -85,6 +85,7 @@ 'storage/http_load.cpp', 'storage/http_other_loop.cpp', 'storage/http_reading.cpp', + 'storage/http_timeout.cpp', 'style/glyph_store.cpp', 'style/pending_resources.cpp', From 4e3503ea6cf30c55a2cc86f78c4a607bd14f1c41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 16 Oct 2015 13:53:43 +0200 Subject: [PATCH 5/8] [core] do not use std::atomic_* with shared_ptrs It's not implemented in GCC 4.9.2's stdlib (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57250). Instead, we're now always using a mutex to protect access; we previously created a mutex only on cancelation, but since we're always canceling now, it makes sense to allocate it right away. --- include/mbgl/storage/request.hpp | 8 ++--- src/mbgl/storage/request.cpp | 51 ++++++++++++++++---------------- 2 files changed, 29 insertions(+), 30 deletions(-) diff --git a/include/mbgl/storage/request.hpp b/include/mbgl/storage/request.hpp index 4b75f23f6e5..1fb15c5a92b 100644 --- a/include/mbgl/storage/request.hpp +++ b/include/mbgl/storage/request.hpp @@ -33,13 +33,13 @@ class Request : private util::noncopyable { private: ~Request(); - void invoke(); void notifyCallback(); private: - std::unique_ptr async; - struct Canceled; - std::unique_ptr canceled; + std::mutex mtx; + bool canceled = false; + bool confirmed = false; + const std::unique_ptr async; Callback callback; std::shared_ptr response; diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index 55913846cf9..5ed9278d0a6 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -6,12 +6,10 @@ #include #include -#include +#include 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, Callback callback_) : async(std::make_unique(loop, [this] { notifyCallback(); })), @@ -21,56 +19,57 @@ Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_) // Called in the originating thread. void Request::notifyCallback() { + std::unique_lock lock(mtx); if (!canceled) { - invoke(); - } else { - bool destroy = false; - { - std::unique_lock lock(canceled->mutex); - destroy = canceled->confirmed; + // Move the response object out so that we can't accidentally notify twice. + auto res = std::move(response); + assert(!response); + // Unlock before, since callbacks may call cancel, which also locks this mutex. + lock.unlock(); + // 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. + // Similarly, two consecutive updates could trigger two notifyCallbacks, so we need to make + // sure + if (callback && res) { + callback(*res); } + } else { // Don't delete right way, because we have to unlock the mutex before deleting. - if (destroy) { + if (confirmed) { + lock.unlock(); delete this; } } } -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(*std::atomic_load(&response)); - } -} - Request::~Request() = default; // Called in the FileSource thread. void Request::notify(const std::shared_ptr &response_) { + std::lock_guard lock(mtx); assert(response_); - std::atomic_store(&response, response_); + response = response_; async->send(); } // Called in the originating thread. void Request::cancel() { - assert(async); + std::lock_guard lock(mtx); assert(!canceled); - canceled = std::make_unique(); + canceled = true; } // Called in the FileSource thread. // Will only ever be invoked after cancel() was called in the original requesting thread. void Request::destruct() { - assert(async); + std::lock_guard lock(mtx); assert(canceled); - std::unique_lock lock(canceled->mutex); - canceled->confirmed = true; + confirmed = true; + // We need to extend the lock until after the async has been sent, otherwise the requesting + // thread could destroy the async while this call is still in progress. async->send(); - // after this method returns, the FileSource thread has no knowledge of + // After this method returns, the FileSource thread has no knowledge of // this object anymore. } From 5173bf1bb8d21054b0dd6251d23eb37323d6c525 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 16 Oct 2015 16:14:55 +0200 Subject: [PATCH 6/8] [core] Make response data shared to avoid excessive copying --- include/mbgl/storage/response.hpp | 2 +- platform/darwin/http_request_nsurl.mm | 5 ++- platform/default/asset_request_fs.cpp | 6 ++- platform/default/asset_request_zip.cpp | 6 ++- platform/default/http_request_curl.cpp | 12 ++++-- platform/default/sqlite_cache.cpp | 15 +++++--- platform/node/src/node_request.cpp | 4 +- src/mbgl/map/map_context.cpp | 2 +- src/mbgl/map/source.cpp | 2 +- src/mbgl/map/sprite.cpp | 20 ++++------ src/mbgl/map/sprite.hpp | 5 --- src/mbgl/map/vector_tile_data.hpp | 2 +- src/mbgl/storage/default_file_source.cpp | 1 - src/mbgl/text/glyph_pbf.cpp | 5 ++- src/mbgl/text/glyph_pbf.hpp | 2 +- src/mbgl/util/worker.cpp | 12 +++--- src/mbgl/util/worker.hpp | 4 +- test/fixtures/mock_file_source.cpp | 7 ++-- test/storage/cache_response.cpp | 6 ++- test/storage/cache_revalidate.cpp | 47 ++++++++---------------- test/storage/database.cpp | 24 +++++++----- test/storage/directory_reading.cpp | 2 +- test/storage/file_reading.cpp | 11 ++++-- test/storage/http_cancel.cpp | 3 +- test/storage/http_coalescing.cpp | 15 +++++--- test/storage/http_error.cpp | 5 ++- test/storage/http_header_parsing.cpp | 6 ++- test/storage/http_issue_1369.cpp | 3 +- test/storage/http_load.cpp | 3 +- test/storage/http_other_loop.cpp | 3 +- test/storage/http_reading.cpp | 7 +++- test/storage/http_timeout.cpp | 3 +- test/storage/server.js | 2 +- 33 files changed, 135 insertions(+), 117 deletions(-) diff --git a/include/mbgl/storage/response.hpp b/include/mbgl/storage/response.hpp index b232cd06f4e..63904260307 100644 --- a/include/mbgl/storage/response.hpp +++ b/include/mbgl/storage/response.hpp @@ -18,7 +18,7 @@ class Response { int64_t modified = 0; int64_t expires = 0; std::string etag; - std::string data; + std::shared_ptr data; }; } diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm index f1f6e3fbb98..f5a4d8ef97c 100644 --- a/platform/darwin/http_request_nsurl.mm +++ b/platform/darwin/http_request_nsurl.mm @@ -221,6 +221,9 @@ int64_t parseCacheControl(const char *value) { // TODO: Use different codes for host not found, timeout, invalid URL etc. // These can be categorized in temporary and permanent errors. response = std::make_unique(); + if (data) { + response->data = std::make_shared((const char *)[data bytes], [data length]); + } response->status = Response::Error; response->message = [[error localizedDescription] UTF8String]; @@ -253,7 +256,7 @@ int64_t parseCacheControl(const char *value) { const long responseCode = [(NSHTTPURLResponse *)res statusCode]; response = std::make_unique(); - response->data = {(const char *)[data bytes], [data length]}; + response->data = std::make_shared((const char *)[data bytes], [data length]); NSDictionary *headers = [(NSHTTPURLResponse *)res allHeaderFields]; NSString *cache_control = [headers objectForKey:@"Cache-Control"]; diff --git a/platform/default/asset_request_fs.cpp b/platform/default/asset_request_fs.cpp index 9fc4e55de49..ac54cda0a11 100644 --- a/platform/default/asset_request_fs.cpp +++ b/platform/default/asset_request_fs.cpp @@ -138,8 +138,10 @@ void AssetRequest::fileStated(uv_fs_t *req) { #endif self->response->etag = util::toString(stat->st_ino); const auto size = (unsigned int)(stat->st_size); - self->response->data.resize(size); - self->buffer = uv_buf_init(const_cast(self->response->data.data()), size); + auto data = std::make_shared(); + self->response->data = data; + data->resize(size); + self->buffer = uv_buf_init(const_cast(data->data()), size); uv_fs_req_cleanup(req); #if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10 uv_fs_read(req->loop, req, self->fd, self->buffer.base, self->buffer.len, -1, fileRead); diff --git a/platform/default/asset_request_zip.cpp b/platform/default/asset_request_zip.cpp index 13c7df4cc80..2947bb3c6ad 100644 --- a/platform/default/asset_request_zip.cpp +++ b/platform/default/asset_request_zip.cpp @@ -180,8 +180,10 @@ void AssetRequest::fileStated(uv_zip_t *zip) { response = std::make_unique(); // Allocate the space for reading the data. - response->data.resize(zip->stat->size); - buffer = uv_buf_init(const_cast(response->data.data()), zip->stat->size); + auto data = std::make_shared(); + data->resize(zip->stat->size); + buffer = uv_buf_init(const_cast(data->data()), zip->stat->size); + response->data = data; // Get the modification time in case we have one. if (zip->stat->valid & ZIP_STAT_MTIME) { diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp index 307993876aa..2d80f0b60af 100644 --- a/platform/default/http_request_curl.cpp +++ b/platform/default/http_request_curl.cpp @@ -111,6 +111,7 @@ class HTTPCURLRequest : public HTTPRequestBase { HTTPCURLContext *context = nullptr; // Will store the current response. + std::shared_ptr data; std::unique_ptr response; // In case of revalidation requests, this will store the old response. @@ -516,11 +517,11 @@ size_t HTTPCURLRequest::writeCallback(void *const contents, const size_t size, c auto impl = reinterpret_cast(userp); MBGL_VERIFY_THREAD(impl->tid); - if (!impl->response) { - impl->response = std::make_unique(); + if (!impl->data) { + impl->data = std::make_shared(); } - impl->response->data.append((char *)contents, size * nmemb); + impl->data->append((char *)contents, size * nmemb); return size * nmemb; } @@ -688,22 +689,27 @@ void HTTPCURLRequest::handleResult(CURLcode code) { // This is an unsolicited 304 response and should only happen on malfunctioning // HTTP servers. It likely doesn't include any data, but we don't have much options. response->status = Response::Successful; + response->data = std::move(data); return finish(ResponseStatus::Successful); } } else if (responseCode == 200) { response->status = Response::Successful; + response->data = std::move(data); return finish(ResponseStatus::Successful); } else if (responseCode == 404) { response->status = Response::NotFound; + response->data = std::move(data); return finish(ResponseStatus::Successful); } else if (responseCode >= 500 && responseCode < 600) { // Server errors may be temporary, so back off exponentially. response->status = Response::Error; + response->data = std::move(data); response->message = "HTTP status code " + util::toString(responseCode); return finish(ResponseStatus::TemporaryError); } else { // We don't know how to handle any other errors, so declare them as permanently failing. response->status = Response::Error; + response->data = std::move(data); response->message = "HTTP status code " + util::toString(responseCode); return finish(ResponseStatus::PermanentError); } diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index 46df7ed2af2..e618c960c48 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -117,9 +117,9 @@ void SQLiteCache::Impl::get(const Resource &resource, Callback callback) { response->modified = getStmt->get(1); response->etag = getStmt->get(2); response->expires = getStmt->get(3); - response->data = getStmt->get(4); + response->data = std::make_shared(std::move(getStmt->get(4))); if (getStmt->get(5)) { // == compressed - response->data = util::decompress(response->data); + response->data = std::make_shared(std::move(util::decompress(*response->data))); } callback(std::move(response)); } else { @@ -171,18 +171,21 @@ void SQLiteCache::Impl::put(const Resource& resource, std::shared_ptrbind(6 /* expires */, response->expires); std::string data; - if (resource.kind != Resource::SpriteImage) { + if (resource.kind != Resource::SpriteImage && response->data) { // Do not compress images, since they are typically compressed already. - data = util::compress(response->data); + data = util::compress(*response->data); } - if (!data.empty() && data.size() < response->data.size()) { + if (!data.empty() && data.size() < response->data->size()) { // Store the compressed data when it is smaller than the original // uncompressed data. putStmt->bind(7 /* data */, data, false); // do not retain the string internally. putStmt->bind(8 /* compressed */, true); + } else if (response->data) { + putStmt->bind(7 /* data */, *response->data, false); // do not retain the string internally. + putStmt->bind(8 /* compressed */, false); } else { - putStmt->bind(7 /* data */, response->data, false); // do not retain the string internally. + putStmt->bind(7 /* data */, "", false); putStmt->bind(8 /* compressed */, false); } diff --git a/platform/node/src/node_request.cpp b/platform/node/src/node_request.cpp index fb4d47045b5..211a696cd74 100644 --- a/platform/node/src/node_request.cpp +++ b/platform/node/src/node_request.cpp @@ -110,10 +110,10 @@ NAN_METHOD(NodeRequest::Respond) { if (Nan::Has(res, Nan::New("data").ToLocalChecked()).FromJust()) { auto dataHandle = Nan::Get(res, Nan::New("data").ToLocalChecked()).ToLocalChecked(); if (node::Buffer::HasInstance(dataHandle)) { - response->data = std::string { + response->data = std::make_shared( node::Buffer::Data(dataHandle), node::Buffer::Length(dataHandle) - }; + ); } else { return Nan::ThrowTypeError("Response data must be a Buffer"); } diff --git a/src/mbgl/map/map_context.cpp b/src/mbgl/map/map_context.cpp index b197ea12f3e..11d631a57a0 100644 --- a/src/mbgl/map/map_context.cpp +++ b/src/mbgl/map/map_context.cpp @@ -113,7 +113,7 @@ void MapContext::setStyleURL(const std::string& url) { styleRequest = nullptr; if (res.status == Response::Successful) { - loadStyleJSON(res.data, base); + loadStyleJSON(*res.data, base); } else if (res.status == Response::NotFound && styleURL.find("mapbox://") == 0) { Log::Error(Event::Setup, "style %s could not be found or is an incompatible legacy map or style", styleURL.c_str()); } else { diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index 7e306828950..551a9eb1909 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -163,7 +163,7 @@ void Source::load() { } rapidjson::Document d; - d.Parse<0>(res.data.c_str()); + d.Parse<0>(res.data->c_str()); if (d.HasParseError()) { std::stringstream message; diff --git a/src/mbgl/map/sprite.cpp b/src/mbgl/map/sprite.cpp index a54d96f005c..cbd8c443386 100644 --- a/src/mbgl/map/sprite.cpp +++ b/src/mbgl/map/sprite.cpp @@ -20,10 +20,8 @@ namespace mbgl { struct Sprite::Loader { - bool loadedJSON = false; - bool loadedImage = false; - std::unique_ptr data = std::make_unique(); - + std::shared_ptr image; + std::shared_ptr json; RequestHolder jsonRequest; RequestHolder spriteRequest; }; @@ -50,8 +48,7 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) } loader->jsonRequest = nullptr; if (res.status == Response::Successful) { - loader->data->json = res.data; - loader->loadedJSON = true; + loader->json = res.data; } else { std::stringstream message; message << "Failed to load [" << jsonURL << "]: " << res.message; @@ -70,8 +67,7 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) } loader->spriteRequest = nullptr; if (res.status == Response::Successful) { - loader->data->image = res.data; - loader->loadedImage = true; + loader->image = res.data; } else { std::stringstream message; message << "Failed to load [" << spriteURL << "]: " << res.message; @@ -88,14 +84,12 @@ Sprite::~Sprite() { void Sprite::emitSpriteLoadedIfComplete() { assert(loader); - if (!loader->loadedImage || !loader->loadedJSON || !observer) { + if (!loader->image || !loader->json || !observer) { return; } - std::unique_ptr data(std::move(loader->data)); - loader.reset(); - - auto result = parseSprite(data->image, data->json); + auto local = std::move(loader); + auto result = parseSprite(*local->image, *local->json); if (result.is()) { loaded = true; observer->onSpriteLoaded(result.get()); diff --git a/src/mbgl/map/sprite.hpp b/src/mbgl/map/sprite.hpp index 32fa16de12b..47ce1dbc8b2 100644 --- a/src/mbgl/map/sprite.hpp +++ b/src/mbgl/map/sprite.hpp @@ -19,11 +19,6 @@ class Request; class Sprite : private util::noncopyable { public: - struct Data { - std::string image; - std::string json; - }; - class Observer { public: virtual ~Observer() = default; diff --git a/src/mbgl/map/vector_tile_data.hpp b/src/mbgl/map/vector_tile_data.hpp index 1a1ff84061a..9b4fa8622bb 100644 --- a/src/mbgl/map/vector_tile_data.hpp +++ b/src/mbgl/map/vector_tile_data.hpp @@ -42,7 +42,7 @@ class VectorTileData : public TileData { bool parsing = false; const SourceInfo& source; RequestHolder req; - std::string data; + std::shared_ptr data; float lastAngle = 0; float currentAngle; float lastPitch = 0; diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 5d698fc0f0d..3e4c94ce40f 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -127,7 +127,6 @@ void DefaultFileSource::Impl::update(DefaultFileRequest* request) { if (!request->response->stale && request->response->isExpired()) { // Create a new Response object with `stale = true`, but the same data, and // replace the current request object we have. - // TODO: Make content shared_ptrs so we won't make copies of the content. auto response = std::make_shared(*request->response); response->stale = true; request->response = response; diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp index f351e66c2a0..66008adb96c 100644 --- a/src/mbgl/text/glyph_pbf.cpp +++ b/src/mbgl/text/glyph_pbf.cpp @@ -98,14 +98,15 @@ GlyphPBF::GlyphPBF(GlyphStore* store, GlyphPBF::~GlyphPBF() = default; void GlyphPBF::parse(GlyphStore* store, const std::string& fontStack, const std::string& url) { - if (data.empty()) { + assert(data); + if (data->empty()) { // If there is no data, this means we either haven't // received any data. return; } try { - parseGlyphPBF(**store->getFontStack(fontStack), std::move(data)); + parseGlyphPBF(**store->getFontStack(fontStack), *data); } catch (const std::exception& ex) { std::stringstream message; message << "Failed to parse [" << url << "]: " << ex.what(); diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp index 205824bfe5b..bf8567dbec6 100644 --- a/src/mbgl/text/glyph_pbf.hpp +++ b/src/mbgl/text/glyph_pbf.hpp @@ -42,7 +42,7 @@ class GlyphPBF : private util::noncopyable { void parse(GlyphStore* store, const std::string& fontStack, const std::string& url); - std::string data; + std::shared_ptr data; std::atomic parsed; RequestHolder req; diff --git a/src/mbgl/util/worker.cpp b/src/mbgl/util/worker.cpp index 71c930b4ef5..fc599713b3d 100644 --- a/src/mbgl/util/worker.cpp +++ b/src/mbgl/util/worker.cpp @@ -16,8 +16,8 @@ class Worker::Impl { public: Impl() = default; - void parseRasterTile(RasterBucket* bucket, std::string data, std::function callback) { - std::unique_ptr image(new util::Image(data)); + void parseRasterTile(RasterBucket* bucket, const std::shared_ptr data, std::function callback) { + std::unique_ptr image(new util::Image(*data)); if (!(*image)) { callback(TileParseResult("error parsing raster image")); } @@ -29,9 +29,9 @@ class Worker::Impl { callback(TileParseResult(TileData::State::parsed)); } - void parseVectorTile(TileWorker* worker, std::string data, std::function callback) { + void parseVectorTile(TileWorker* worker, const std::shared_ptr data, std::function callback) { try { - pbf tilePBF(reinterpret_cast(data.data()), data.size()); + pbf tilePBF(reinterpret_cast(data->data()), data->size()); callback(worker->parse(VectorTile(tilePBF))); } catch (const std::exception& ex) { callback(TileParseResult(ex.what())); @@ -61,12 +61,12 @@ Worker::Worker(std::size_t count) { Worker::~Worker() = default; -std::unique_ptr Worker::parseRasterTile(RasterBucket& bucket, std::string data, std::function callback) { +std::unique_ptr Worker::parseRasterTile(RasterBucket& bucket, const std::shared_ptr data, std::function callback) { current = (current + 1) % threads.size(); return threads[current]->invokeWithCallback(&Worker::Impl::parseRasterTile, callback, &bucket, data); } -std::unique_ptr Worker::parseVectorTile(TileWorker& worker, std::string data, std::function callback) { +std::unique_ptr Worker::parseVectorTile(TileWorker& worker, const std::shared_ptr data, std::function callback) { current = (current + 1) % threads.size(); return threads[current]->invokeWithCallback(&Worker::Impl::parseVectorTile, callback, &worker, data); } diff --git a/src/mbgl/util/worker.hpp b/src/mbgl/util/worker.hpp index 4e63b45abfd..18b1bc92b1a 100644 --- a/src/mbgl/util/worker.hpp +++ b/src/mbgl/util/worker.hpp @@ -33,12 +33,12 @@ class Worker : public mbgl::util::noncopyable { Request parseRasterTile( RasterBucket&, - std::string data, + std::shared_ptr data, std::function callback); Request parseVectorTile( TileWorker&, - std::string data, + std::shared_ptr data, std::function callback); Request parseLiveTile( diff --git a/test/fixtures/mock_file_source.cpp b/test/fixtures/mock_file_source.cpp index b420ba7298a..407f54408f1 100644 --- a/test/fixtures/mock_file_source.cpp +++ b/test/fixtures/mock_file_source.cpp @@ -55,7 +55,7 @@ void MockFileSource::Impl::replyWithSuccess(Request* req) const { res->status = Response::Status::Successful; try { - res->data = util::read_file(req->resource.url); + res->data = std::make_shared(std::move(util::read_file(req->resource.url))); } catch (const std::exception& err) { res->status = Response::Status::Error; res->message = err.what(); @@ -95,8 +95,9 @@ void MockFileSource::Impl::replyWithCorruptedData(Request* req) const { std::shared_ptr res = std::make_shared(); res->status = Response::Status::Successful; - res->data = util::read_file(req->resource.url); - res->data.insert(0, "CORRUPTED"); + auto data = std::make_shared(std::move(util::read_file(req->resource.url))); + data->insert(0, "CORRUPTED"); + res->data = std::move(data); req->notify(res); } diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp index 12945138587..5b0923900e7 100644 --- a/test/storage/cache_response.cpp +++ b/test/storage/cache_response.cpp @@ -20,7 +20,8 @@ TEST_F(Storage, CacheResponse) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Response 1", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Response 1", *res.data); EXPECT_LT(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); @@ -36,7 +37,8 @@ TEST_F(Storage, CacheResponse) { fs.cancel(req); EXPECT_EQ(response.status, res.status); EXPECT_EQ(response.stale, res.stale); - EXPECT_EQ(response.data, res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ(*response.data, *res.data); EXPECT_EQ(response.expires, res.expires); EXPECT_EQ(response.modified, res.modified); EXPECT_EQ(response.etag, res.etag); diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp index 90633b83ac5..d7dcbe71cda 100644 --- a/test/storage/cache_revalidate.cpp +++ b/test/storage/cache_revalidate.cpp @@ -13,8 +13,6 @@ TEST_F(Storage, CacheRevalidateSame) { SQLiteCache cache(":memory:"); DefaultFileSource fs(&cache); - const Response *reference = nullptr; - const Resource revalidateSame { Resource::Unknown, "http://127.0.0.1:3000/revalidate-same" }; Request* req1 = nullptr; Request* req2 = nullptr; @@ -27,21 +25,16 @@ TEST_F(Storage, CacheRevalidateSame) { } first = false; - EXPECT_EQ(nullptr, reference); - reference = &res; - EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Response", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Response", *res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("snowfall", res.etag); EXPECT_EQ("", res.message); req2 = fs.request(revalidateSame, uv_default_loop(), [&, res](const Response &res2) { - // Make sure we get a different object than before, since this request should've been revalidated. - EXPECT_TRUE(reference != &res2); - if (res2.stale) { // Discard stale responses, if any. return; @@ -57,7 +50,9 @@ TEST_F(Storage, CacheRevalidateSame) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Response", res2.data); + ASSERT_TRUE(res2.data.get()); + EXPECT_EQ(res.data, res2.data); + EXPECT_EQ("Response", *res2.data); // We use this to indicate that a 304 reply came back. EXPECT_LT(0, res2.expires); EXPECT_EQ(0, res2.modified); @@ -80,8 +75,6 @@ TEST_F(Storage, CacheRevalidateModified) { SQLiteCache cache(":memory:"); DefaultFileSource fs(&cache); - const Response *reference = nullptr; - const Resource revalidateModified{ Resource::Unknown, "http://127.0.0.1:3000/revalidate-modified" }; Request* req1 = nullptr; @@ -95,21 +88,16 @@ TEST_F(Storage, CacheRevalidateModified) { } first = false; - EXPECT_EQ(nullptr, reference); - reference = &res; - EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Response", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Response", *res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(1420070400, res.modified); EXPECT_EQ("", res.etag); EXPECT_EQ("", res.message); req2 = fs.request(revalidateModified, uv_default_loop(), [&, res](const Response &res2) { - // Make sure we get a different object than before, since this request should've been revalidated. - EXPECT_TRUE(reference != &res2); - if (res2.stale) { // Discard stale responses, if any. return; @@ -124,8 +112,10 @@ TEST_F(Storage, CacheRevalidateModified) { req2 = nullptr; EXPECT_EQ(Response::Successful, res2.status); - EXPECT_EQ(false, res.stale); - EXPECT_EQ("Response", res2.data); + EXPECT_EQ(false, res2.stale); + ASSERT_TRUE(res2.data.get()); + EXPECT_EQ("Response", *res2.data); + EXPECT_EQ(res.data, res2.data); // We use this to indicate that a 304 reply came back. EXPECT_LT(0, res2.expires); EXPECT_EQ(1420070400, res2.modified); @@ -147,8 +137,6 @@ TEST_F(Storage, CacheRevalidateEtag) { SQLiteCache cache(":memory:"); DefaultFileSource fs(&cache); - const Response *reference = nullptr; - const Resource revalidateEtag { Resource::Unknown, "http://127.0.0.1:3000/revalidate-etag" }; Request* req1 = nullptr; Request* req2 = nullptr; @@ -161,21 +149,16 @@ TEST_F(Storage, CacheRevalidateEtag) { } first = false; - EXPECT_EQ(nullptr, reference); - reference = &res; - EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Response 1", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Response 1", *res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("response-1", res.etag); EXPECT_EQ("", res.message); req2 = fs.request(revalidateEtag, uv_default_loop(), [&, res](const Response &res2) { - // Make sure we get a different object than before, since this request should've been revalidated. - EXPECT_TRUE(reference != &res2); - if (res2.stale) { // Discard stale responses, if any. return; @@ -191,7 +174,9 @@ TEST_F(Storage, CacheRevalidateEtag) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Response 2", res2.data); + ASSERT_TRUE(res2.data.get()); + EXPECT_NE(res.data, res2.data); + EXPECT_EQ("Response 2", *res2.data); EXPECT_EQ(0, res2.expires); EXPECT_EQ(0, res2.modified); EXPECT_EQ("response-2", res2.etag); diff --git a/test/storage/database.cpp b/test/storage/database.cpp index 20f96305b13..dd186f2a3f7 100644 --- a/test/storage/database.cpp +++ b/test/storage/database.cpp @@ -178,11 +178,12 @@ TEST_F(Storage, DatabaseLockedWrite) { Log::setObserver(std::make_unique()); auto response = std::make_shared(); - response->data = "Demo"; + response->data = std::make_shared("Demo"); cache.put({ Resource::Unknown, "mapbox://test" }, response); cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr res) { ASSERT_NE(nullptr, res.get()); - EXPECT_EQ("Demo", res->data); + ASSERT_TRUE(res->data.get()); + EXPECT_EQ("Demo", *res->data); }); // Make sure that we got a no errors @@ -210,7 +211,7 @@ TEST_F(Storage, DatabaseLockedRefresh) { Log::setObserver(std::make_unique()); auto response = std::make_shared(); - response->data = "Demo"; + response->data = std::make_shared("Demo"); cache.put({ Resource::Unknown, "mapbox://test" }, response); cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); @@ -226,7 +227,7 @@ TEST_F(Storage, DatabaseLockedRefresh) { Log::setObserver(std::make_unique()); auto response = std::make_shared(); - response->data = "Demo"; + response->data = std::make_shared("Demo"); cache.refresh({ Resource::Unknown, "mapbox://test" }, response->expires); cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); @@ -255,11 +256,12 @@ TEST_F(Storage, DatabaseDeleted) { Log::setObserver(std::make_unique()); auto response = std::make_shared(); - response->data = "Demo"; + response->data = std::make_shared("Demo"); cache.put({ Resource::Unknown, "mapbox://test" }, response); cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr res) { ASSERT_NE(nullptr, res.get()); - EXPECT_EQ("Demo", res->data); + ASSERT_TRUE(res->data.get()); + EXPECT_EQ("Demo", *res->data); }); Log::removeObserver(); @@ -272,11 +274,12 @@ TEST_F(Storage, DatabaseDeleted) { Log::setObserver(std::make_unique()); auto response = std::make_shared(); - response->data = "Demo"; + response->data = std::make_shared("Demo"); cache.put({ Resource::Unknown, "mapbox://test" }, response); cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr res) { ASSERT_NE(nullptr, res.get()); - EXPECT_EQ("Demo", res->data); + ASSERT_TRUE(res->data.get()); + EXPECT_EQ("Demo", *res->data); }); auto observer = Log::removeObserver(); @@ -302,11 +305,12 @@ TEST_F(Storage, DatabaseInvalid) { Log::setObserver(std::make_unique()); auto response = std::make_shared(); - response->data = "Demo"; + response->data = std::make_shared("Demo"); cache.put({ Resource::Unknown, "mapbox://test" }, response); cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr res) { ASSERT_NE(nullptr, res.get()); - EXPECT_EQ("Demo", res->data); + ASSERT_TRUE(res->data.get()); + EXPECT_EQ("Demo", *res->data); }); auto observer = Log::removeObserver(); diff --git a/test/storage/directory_reading.cpp b/test/storage/directory_reading.cpp index f0bc7ea6d2b..f1012527618 100644 --- a/test/storage/directory_reading.cpp +++ b/test/storage/directory_reading.cpp @@ -20,7 +20,7 @@ TEST_F(Storage, AssetReadDirectory) { fs.cancel(req); EXPECT_EQ(Response::Error, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ(0ul, res.data.size()); + ASSERT_FALSE(res.data.get()); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); diff --git a/test/storage/file_reading.cpp b/test/storage/file_reading.cpp index 434af703f2d..ab7ba326adf 100644 --- a/test/storage/file_reading.cpp +++ b/test/storage/file_reading.cpp @@ -21,7 +21,8 @@ TEST_F(Storage, AssetEmptyFile) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ(0ul, res.data.size()); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("", *res.data); EXPECT_EQ(0, res.expires); EXPECT_LT(1420000000, res.modified); EXPECT_NE("", res.etag); @@ -48,12 +49,14 @@ TEST_F(Storage, AssetNonEmptyFile) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ(16ul, res.data.size()); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("content is here\n", *res.data); EXPECT_EQ(0, res.expires); EXPECT_LT(1420000000, res.modified); EXPECT_NE("", res.etag); EXPECT_EQ("", res.message); - EXPECT_EQ("content is here\n", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("content is here\n", *res.data); NonEmptyFile.finish(); }); @@ -76,7 +79,7 @@ TEST_F(Storage, AssetNonExistentFile) { fs.cancel(req); EXPECT_EQ(Response::Error, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ(0ul, res.data.size()); + ASSERT_FALSE(res.data.get()); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); diff --git a/test/storage/http_cancel.cpp b/test/storage/http_cancel.cpp index 5743ccb8f77..7dfe9c58d52 100644 --- a/test/storage/http_cancel.cpp +++ b/test/storage/http_cancel.cpp @@ -40,7 +40,8 @@ TEST_F(Storage, HTTPCancelMultiple) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); diff --git a/test/storage/http_coalescing.cpp b/test/storage/http_coalescing.cpp index 85583620343..2748fb87576 100644 --- a/test/storage/http_coalescing.cpp +++ b/test/storage/http_coalescing.cpp @@ -27,7 +27,8 @@ TEST_F(Storage, HTTPCoalescing) { } EXPECT_EQ(Response::Successful, res.status); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); @@ -69,7 +70,8 @@ TEST_F(Storage, HTTPMultiple) { // Do not cancel the request right away. EXPECT_EQ(Response::Successful, res.status); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); EXPECT_EQ(2147483647, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); @@ -85,7 +87,8 @@ TEST_F(Storage, HTTPMultiple) { fs.cancel(req2); EXPECT_EQ(Response::Successful, res2.status); - EXPECT_EQ("Hello World!", res2.data); + ASSERT_TRUE(res2.data.get()); + EXPECT_EQ("Hello World!", *res2.data); EXPECT_EQ(2147483647, res2.expires); EXPECT_EQ(0, res2.modified); EXPECT_EQ("", res2.etag); @@ -115,7 +118,8 @@ TEST_F(Storage, HTTPStale) { req1 = fs.request(resource, uv_default_loop(), [&] (const Response &res) { // Do not cancel the request right away. EXPECT_EQ(Response::Successful, res.status); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); EXPECT_EQ(false, res.stale); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); @@ -132,7 +136,8 @@ TEST_F(Storage, HTTPStale) { // Start a second request for the same resource after the first one has been completed. req2 = fs.request(resource, uv_default_loop(), [&] (const Response &res2) { EXPECT_EQ(Response::Successful, res2.status); - EXPECT_EQ("Hello World!", res2.data); + ASSERT_TRUE(res2.data.get()); + EXPECT_EQ("Hello World!", *res2.data); EXPECT_EQ(0, res2.expires); EXPECT_EQ(0, res2.modified); EXPECT_EQ("", res2.etag); diff --git a/test/storage/http_error.cpp b/test/storage/http_error.cpp index 50b46a41b8b..d0eefb408c2 100644 --- a/test/storage/http_error.cpp +++ b/test/storage/http_error.cpp @@ -38,7 +38,8 @@ TEST_F(Storage, HTTPError) { EXPECT_GT(1.2, duration) << "Backoff timer fired too late"; EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); @@ -67,7 +68,7 @@ TEST_F(Storage, HTTPError) { #else FAIL(); #endif - EXPECT_EQ("", res.data); + ASSERT_FALSE(res.data.get()); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); diff --git a/test/storage/http_header_parsing.cpp b/test/storage/http_header_parsing.cpp index 93fdcb62313..17c3ce16cf7 100644 --- a/test/storage/http_header_parsing.cpp +++ b/test/storage/http_header_parsing.cpp @@ -21,7 +21,8 @@ TEST_F(Storage, HTTPHeaderParsing) { fs.cancel(req1); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); EXPECT_EQ(1420797926, res.expires); EXPECT_EQ(1420794326, res.modified); EXPECT_EQ("foo", res.etag); @@ -37,7 +38,8 @@ TEST_F(Storage, HTTPHeaderParsing) { fs.cancel(req2); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + 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"; EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); diff --git a/test/storage/http_issue_1369.cpp b/test/storage/http_issue_1369.cpp index ba4e3d38851..c5135b3ab9b 100644 --- a/test/storage/http_issue_1369.cpp +++ b/test/storage/http_issue_1369.cpp @@ -34,7 +34,8 @@ TEST_F(Storage, HTTPIssue1369) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); diff --git a/test/storage/http_load.cpp b/test/storage/http_load.cpp index 092bf2db5a5..987e4635079 100644 --- a/test/storage/http_load.cpp +++ b/test/storage/http_load.cpp @@ -26,7 +26,8 @@ TEST_F(Storage, HTTPLoad) { reqs[i] = nullptr; EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ(std::string("Request ") + std::to_string(current), res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ(std::string("Request ") + std::to_string(current), *res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); diff --git a/test/storage/http_other_loop.cpp b/test/storage/http_other_loop.cpp index cd9fad95be9..f6e805285f5 100644 --- a/test/storage/http_other_loop.cpp +++ b/test/storage/http_other_loop.cpp @@ -17,7 +17,8 @@ TEST_F(Storage, HTTPOtherLoop) { fs.cancel(req); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp index c5e89b88e88..468ba332482 100644 --- a/test/storage/http_reading.cpp +++ b/test/storage/http_reading.cpp @@ -24,7 +24,8 @@ TEST_F(Storage, HTTPReading) { EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); @@ -38,6 +39,8 @@ TEST_F(Storage, HTTPReading) { EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::NotFound, res.status); EXPECT_EQ(false, res.stale); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Cannot GET /doesnotexist\n", *res.data); EXPECT_EQ("", res.message); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); @@ -51,6 +54,8 @@ TEST_F(Storage, HTTPReading) { EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Error, res.status); EXPECT_EQ(false, res.stale); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Server Error!", *res.data); EXPECT_EQ("HTTP status code 500", res.message); EXPECT_EQ(0, res.expires); EXPECT_EQ(0, res.modified); diff --git a/test/storage/http_timeout.cpp b/test/storage/http_timeout.cpp index 71553677bd2..b5cd877e762 100644 --- a/test/storage/http_timeout.cpp +++ b/test/storage/http_timeout.cpp @@ -20,7 +20,8 @@ TEST_F(Storage, HTTPTimeout) { counter++; EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(false, res.stale); - EXPECT_EQ("Hello World!", res.data); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); EXPECT_LT(0, res.expires); EXPECT_EQ(0, res.modified); EXPECT_EQ("", res.etag); diff --git a/test/storage/server.js b/test/storage/server.js index 342a61cdc14..efc1e1d6c0b 100755 --- a/test/storage/server.js +++ b/test/storage/server.js @@ -74,7 +74,7 @@ app.get('/revalidate-etag', function(req, res) { }); app.get('/permanent-error', function(req, res) { - res.status(500).end(); + res.status(500).send('Server Error!'); }); var temporaryErrorCounter = 0; From c0c554e36fd43bfe57ef13fe60f9cd50b5c018fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Tue, 20 Oct 2015 12:19:11 +0200 Subject: [PATCH 7/8] [core] reparse tiles when new data arrives We're now reparsing tiles when they expire. We're also swapping out buckets atomically to avoid flickering data; i.e. we're displaying the old data as long as we don't have a new parsed bucket for that layer yet. The parsed buckets now live in the *TileData objects rather than in the TileWorker; only partially parsed == pending buckets will remain in the TileWorker. Once they're parsed, they're moved to the *TileData object. --- include/mbgl/storage/response.hpp | 1 + src/mbgl/map/live_tile_data.cpp | 31 +++- src/mbgl/map/live_tile_data.hpp | 7 +- src/mbgl/map/raster_tile_data.cpp | 25 ++- src/mbgl/map/raster_tile_data.hpp | 3 +- src/mbgl/map/source.cpp | 12 +- src/mbgl/map/source.hpp | 2 +- src/mbgl/map/tile_data.cpp | 23 ++- src/mbgl/map/tile_data.hpp | 16 +- src/mbgl/map/tile_worker.cpp | 245 +++++++++++++++------------- src/mbgl/map/tile_worker.hpp | 51 +++--- src/mbgl/map/vector_tile_data.cpp | 99 ++++++++--- src/mbgl/map/vector_tile_data.hpp | 23 ++- src/mbgl/renderer/bucket.hpp | 2 + src/mbgl/renderer/circle_bucket.hpp | 2 +- src/mbgl/renderer/debug_bucket.cpp | 16 +- src/mbgl/renderer/debug_bucket.hpp | 15 +- src/mbgl/renderer/fill_bucket.hpp | 2 +- src/mbgl/renderer/line_bucket.hpp | 2 +- src/mbgl/renderer/painter.hpp | 2 +- src/mbgl/renderer/painter_debug.cpp | 14 +- src/mbgl/renderer/raster_bucket.hpp | 2 +- src/mbgl/renderer/symbol_bucket.cpp | 17 +- src/mbgl/renderer/symbol_bucket.hpp | 12 +- src/mbgl/util/worker.cpp | 84 ++++++++-- src/mbgl/util/worker.hpp | 37 ++--- test/style/resource_loading.cpp | 5 + 27 files changed, 464 insertions(+), 286 deletions(-) diff --git a/include/mbgl/storage/response.hpp b/include/mbgl/storage/response.hpp index 63904260307..b5973457b51 100644 --- a/include/mbgl/storage/response.hpp +++ b/include/mbgl/storage/response.hpp @@ -2,6 +2,7 @@ #define MBGL_STORAGE_RESPONSE #include +#include namespace mbgl { diff --git a/src/mbgl/map/live_tile_data.cpp b/src/mbgl/map/live_tile_data.cpp index f6e14fad10c..fb8e6c3605a 100644 --- a/src/mbgl/map/live_tile_data.cpp +++ b/src/mbgl/map/live_tile_data.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include @@ -32,21 +33,27 @@ LiveTileData::LiveTileData(const TileID& id_, return; } - reparse(callback); + parsePending(callback); } -bool LiveTileData::reparse(std::function callback) { - if (parsing || (state != State::loaded && state != State::partial)) { +bool LiveTileData::parsePending(std::function callback) { + if (workRequest || (state != State::loaded && state != State::partial)) { return false; } - parsing = true; - workRequest = worker.parseLiveTile(tileWorker, *tile, [this, callback] (TileParseResult result) { - parsing = false; + workRequest.reset(); + + if (result.is()) { + auto& resultBuckets = result.get(); + state = resultBuckets.state; + + // Move over all buckets we received in this parse request, potentially overwriting + // existing buckets in case we got a refresh parse. + for (auto& bucket : resultBuckets.buckets) { + buckets[bucket.first] = std::move(bucket.second); + } - if (result.is()) { - state = result.get(); } else { error = result.get(); state = State::obsolete; @@ -67,7 +74,13 @@ Bucket* LiveTileData::getBucket(const StyleLayer& layer) { return nullptr; } - return tileWorker.getBucket(layer); + const auto it = buckets.find(layer.bucket->name); + if (it == buckets.end()) { + return nullptr; + } + + assert(it->second); + return it->second.get(); } void LiveTileData::cancel() { diff --git a/src/mbgl/map/live_tile_data.hpp b/src/mbgl/map/live_tile_data.hpp index 6be9fa73df1..568892216c4 100644 --- a/src/mbgl/map/live_tile_data.hpp +++ b/src/mbgl/map/live_tile_data.hpp @@ -20,7 +20,7 @@ class LiveTileData : public TileData { std::function callback); ~LiveTileData(); - bool reparse(std::function callback) override; + bool parsePending(std::function callback) override; void cancel() override; Bucket* getBucket(const StyleLayer&) override; @@ -29,8 +29,11 @@ class LiveTileData : public TileData { Worker& worker; TileWorker tileWorker; std::unique_ptr workRequest; - bool parsing = false; std::unique_ptr tile; + + // Contains all the Bucket objects for the tile. Buckets are render + // objects and they get added by tile parsing operations. + std::unordered_map> buckets; }; } diff --git a/src/mbgl/map/raster_tile_data.cpp b/src/mbgl/map/raster_tile_data.cpp index cc7b6b548fe..16f727bac7e 100644 --- a/src/mbgl/map/raster_tile_data.cpp +++ b/src/mbgl/map/raster_tile_data.cpp @@ -11,13 +11,13 @@ using namespace mbgl; RasterTileData::RasterTileData(const TileID& id_, - TexturePool &texturePool, + TexturePool &texturePool_, const SourceInfo &source_, Worker& worker_) : TileData(id_), + texturePool(texturePool_), source(source_), - worker(worker_), - bucket(texturePool, layout) { + worker(worker_) { } RasterTileData::~RasterTileData() { @@ -52,15 +52,24 @@ void RasterTileData::request(float pixelRatio, return; } - state = State::loaded; + if (state == State::loading) { + // Only overwrite the state when we didn't have a previous tile. + state = State::loaded; + } - workRequest = worker.parseRasterTile(bucket, res.data, [this, callback] (TileParseResult result) { + workRequest = worker.parseRasterTile(std::make_unique(texturePool, layout), res.data, [this, callback] (TileParseResult result) { + workRequest.reset(); if (state != State::loaded) { return; } - if (result.is()) { - state = result.get(); + if (result.is()) { + auto& buckets = result.get(); + state = buckets.state; + // TODO: Make this less awkward; we're only getting one bucket back. + if (!buckets.buckets.empty()) { + bucket = std::move(buckets.buckets.front().second); + } } else { std::stringstream message; message << "Failed to parse [" << std::string(id) << "]: " << result.get(); @@ -74,7 +83,7 @@ void RasterTileData::request(float pixelRatio, } Bucket* RasterTileData::getBucket(StyleLayer const&) { - return &bucket; + return bucket.get(); } void RasterTileData::cancel() { diff --git a/src/mbgl/map/raster_tile_data.hpp b/src/mbgl/map/raster_tile_data.hpp index 5c8b42ef961..64bcb777dba 100644 --- a/src/mbgl/map/raster_tile_data.hpp +++ b/src/mbgl/map/raster_tile_data.hpp @@ -27,12 +27,13 @@ class RasterTileData : public TileData { Bucket* getBucket(StyleLayer const &layer_desc) override; private: + TexturePool& texturePool; const SourceInfo& source; Worker& worker; RequestHolder req; RasterLayoutProperties layout; - RasterBucket bucket; + std::unique_ptr bucket; std::unique_ptr workRequest; }; diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index 551a9eb1909..c79ef4b3b68 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -242,7 +242,7 @@ bool Source::handlePartialTile(const TileID& id, Worker&) { return true; } - return data->reparse([this]() { + return data->parsePending([this]() { emitTileLoaded(false); }); } @@ -364,7 +364,8 @@ bool Source::findLoadedChildren(const TileID& id, int32_t maxCoveringZoom, std:: const TileData::State state = hasTile(child_id); if (TileData::isReadyState(state)) { retain.emplace_front(child_id); - } else { + } + if (state != TileData::State::parsed) { complete = false; if (z < maxCoveringZoom) { // Go further down the hierarchy to find more unloaded children. @@ -384,16 +385,17 @@ bool Source::findLoadedChildren(const TileID& id, int32_t maxCoveringZoom, std:: * * @return boolean Whether a parent was found. */ -bool Source::findLoadedParent(const TileID& id, int32_t minCoveringZoom, std::forward_list& retain) { +void Source::findLoadedParent(const TileID& id, int32_t minCoveringZoom, std::forward_list& retain) { for (int32_t z = id.z - 1; z >= minCoveringZoom; --z) { const TileID parent_id = id.parent(z, info.max_zoom); const TileData::State state = hasTile(parent_id); if (TileData::isReadyState(state)) { retain.emplace_front(parent_id); - return true; + if (state == TileData::State::parsed) { + return; + } } } - return false; } bool Source::update(MapData& data, diff --git a/src/mbgl/map/source.hpp b/src/mbgl/map/source.hpp index f5b95ef8741..2193ea8af61 100644 --- a/src/mbgl/map/source.hpp +++ b/src/mbgl/map/source.hpp @@ -105,7 +105,7 @@ class Source : private util::noncopyable { bool handlePartialTile(const TileID &id, Worker &worker); bool findLoadedChildren(const TileID& id, int32_t maxCoveringZoom, std::forward_list& retain); - bool findLoadedParent(const TileID& id, int32_t minCoveringZoom, std::forward_list& retain); + void findLoadedParent(const TileID& id, int32_t minCoveringZoom, std::forward_list& retain); int32_t coveringZoomLevel(const TransformState&) const; std::forward_list coveringTiles(const TransformState&) const; diff --git a/src/mbgl/map/tile_data.cpp b/src/mbgl/map/tile_data.cpp index 453233211ce..d5fef9d0b41 100644 --- a/src/mbgl/map/tile_data.cpp +++ b/src/mbgl/map/tile_data.cpp @@ -1,11 +1,26 @@ #include +#include -using namespace mbgl; +namespace mbgl { TileData::TileData(const TileID& id_) : id(id_), - debugBucket(debugFontBuffer), state(State::initial) { - // Initialize tile debug coordinates - debugFontBuffer.addText(std::string(id).c_str(), 50, 200, 5); } + +TileData::~TileData() = default; + +const char* TileData::StateToString(const State state) { + switch (state) { + case TileData::State::initial: return "initial"; + case TileData::State::invalid : return "invalid"; + case TileData::State::loading : return "loading"; + case TileData::State::loaded : return "loaded"; + case TileData::State::obsolete : return "obsolete"; + case TileData::State::parsed : return "parsed"; + case TileData::State::partial : return "partial"; + default: return ""; + } +} + +} // namespace mbgl diff --git a/src/mbgl/map/tile_data.hpp b/src/mbgl/map/tile_data.hpp index fcbf25fb629..44f0304c8d9 100644 --- a/src/mbgl/map/tile_data.hpp +++ b/src/mbgl/map/tile_data.hpp @@ -3,17 +3,18 @@ #include #include -#include -#include +#include #include #include +#include #include namespace mbgl { class StyleLayer; class Worker; +class DebugBucket; class TileData : private util::noncopyable { public: @@ -57,6 +58,8 @@ class TileData : private util::noncopyable { obsolete }; + static const char* StateToString(State); + // Tile data considered "Ready" can be used for rendering. Data in // partial state is still waiting for network resources but can also // be rendered, although layers will be missing. @@ -65,14 +68,14 @@ class TileData : private util::noncopyable { } TileData(const TileID&); - virtual ~TileData() = default; + virtual ~TileData(); // Mark this tile as no longer needed and cancel any pending work. virtual void cancel() = 0; virtual Bucket* getBucket(const StyleLayer&) = 0; - virtual bool reparse(std::function) { return true; } + virtual bool parsePending(std::function) { return true; } virtual void redoPlacement(float, float, bool) {} bool isReady() const { @@ -90,14 +93,13 @@ class TileData : private util::noncopyable { const TileID id; // Contains the tile ID string for painting debug information. - DebugBucket debugBucket; - DebugFontBuffer debugFontBuffer; + std::unique_ptr debugBucket; protected: std::atomic state; std::string error; }; -} +} // namespace mbgl #endif diff --git a/src/mbgl/map/tile_worker.cpp b/src/mbgl/map/tile_worker.cpp index 45afaee49e0..c7836c4ba94 100644 --- a/src/mbgl/map/tile_worker.cpp +++ b/src/mbgl/map/tile_worker.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -10,6 +11,7 @@ #include #include #include +#include using namespace mbgl; @@ -33,34 +35,66 @@ TileWorker::~TileWorker() { style.glyphAtlas->removeGlyphs(reinterpret_cast(this)); } -Bucket* TileWorker::getBucket(const StyleLayer& layer) const { - std::lock_guard lock(bucketsMutex); +TileParseResult TileWorker::parseAllLayers(const GeometryTile& geometryTile) { + // We're doing a fresh parse of the tile, because the underlying data has changed. + pending.clear(); - const auto it = buckets.find(layer.bucket->name); - if (it == buckets.end()) { - return nullptr; + // We're storing a list of buckets we've parsed to avoid parsing a bucket twice that is + // referenced from more than one layer + std::set parsed; + + for (auto i = layers.rbegin(); i != layers.rend(); i++) { + const StyleLayer& layer = **i; + if (layer.bucket && parsed.find(layer.bucket.get()) == parsed.end()) { + parsed.emplace(layer.bucket.get()); + parseLayer(layer, geometryTile); + } } - assert(it->second); - return it->second.get(); + result.state = pending.empty() ? TileData::State::parsed : TileData::State::partial; + return std::move(result); } -TileParseResult TileWorker::parse(const GeometryTile& geometryTile) { - partialParse = false; +TileParseResult TileWorker::parsePendingLayers() { + // Try parsing the remaining layers that we couldn't parse in the first step due to missing + // dependencies. + for (auto it = pending.begin(); it != pending.end();) { + auto& styleBucket = it->first; + auto& bucket = it->second; + assert(bucket); + + if (styleBucket.type == StyleLayerType::Symbol) { + auto symbolBucket = dynamic_cast(bucket.get()); + if (!symbolBucket->needsDependencies(*style.glyphStore, *style.sprite)) { + symbolBucket->addFeatures(reinterpret_cast(this), *style.spriteAtlas, + *style.glyphAtlas, *style.glyphStore, *collisionTile); + insertBucket(styleBucket.name, std::move(bucket)); + pending.erase(it++); + continue; + } + } - for (auto i = layers.rbegin(); i != layers.rend(); i++) { - parseLayer(**i, geometryTile); + // Advance the iterator here; we're skipping this when erasing an element from this list. + ++it; } - return partialParse ? TileData::State::partial : TileData::State::parsed; + result.state = pending.empty() ? TileData::State::parsed : TileData::State::partial; + return std::move(result); } -void TileWorker::redoPlacement(float angle, float pitch, bool collisionDebug) { +void TileWorker::redoPlacement( + const std::unordered_map>* buckets, + float angle, + float pitch, + bool collisionDebug) { + + // Reset the collision tile so we have a clean slate; we're placing all features anyway. collisionTile = std::make_unique(angle, pitch, collisionDebug); + for (auto i = layers.rbegin(); i != layers.rend(); i++) { - auto bucket = getBucket(**i); - if (bucket) { - bucket->placeFeatures(*collisionTile); + const auto it = buckets->find((*i)->id); + if (it != buckets->end()) { + it->second->placeFeatures(*collisionTile); } } } @@ -88,21 +122,15 @@ void TileWorker::parseLayer(const StyleLayer& layer, const GeometryTile& geometr return; } - // This is a singular layer. Check if this bucket already exists. - if (getBucket(layer)) - return; - const StyleBucket& styleBucket = *layer.bucket; // Skip this bucket if we are to not render this - if (styleBucket.source != sourceID) - return; - if (id.z < std::floor(styleBucket.min_zoom)) - return; - if (id.z >= std::ceil(styleBucket.max_zoom)) - return; - if (styleBucket.visibility == mbgl::VisibilityType::None) + if ((styleBucket.source != sourceID) || + (id.z < std::floor(styleBucket.min_zoom)) || + (id.z >= std::ceil(styleBucket.max_zoom)) || + (styleBucket.visibility == VisibilityType::None)) { return; + } auto geometryLayer = geometryTile.getLayer(styleBucket.source_layer); if (!geometryLayer) { @@ -114,35 +142,25 @@ void TileWorker::parseLayer(const StyleLayer& layer, const GeometryTile& geometr return; } - std::unique_ptr bucket; - switch (styleBucket.type) { case StyleLayerType::Fill: - bucket = createFillBucket(*geometryLayer, styleBucket); + createFillBucket(*geometryLayer, styleBucket); break; case StyleLayerType::Line: - bucket = createLineBucket(*geometryLayer, styleBucket); + createLineBucket(*geometryLayer, styleBucket); break; case StyleLayerType::Circle: - bucket = createCircleBucket(*geometryLayer, styleBucket); + createCircleBucket(*geometryLayer, styleBucket); break; case StyleLayerType::Symbol: - bucket = createSymbolBucket(*geometryLayer, styleBucket); + createSymbolBucket(*geometryLayer, styleBucket); break; case StyleLayerType::Raster: - return; + break; default: Log::Warning(Event::ParseTile, "unknown bucket render type for layer '%s' (source layer '%s')", styleBucket.name.c_str(), styleBucket.source_layer.c_str()); } - - // Bucket creation might fail because the data tile may not - // contain any data that falls into this bucket. - if (!bucket) - return; - - std::lock_guard lock(bucketsMutex); - buckets[styleBucket.name] = std::move(bucket); } template @@ -161,101 +179,108 @@ void TileWorker::addBucketGeometries(Bucket& bucket, const GeometryTileLayer& la } } -std::unique_ptr TileWorker::createFillBucket(const GeometryTileLayer& layer, - const StyleBucket& bucket_desc) { +void TileWorker::createFillBucket(const GeometryTileLayer& layer, + const StyleBucket& styleBucket) { auto bucket = std::make_unique(); - addBucketGeometries(bucket, layer, bucket_desc.filter); - return bucket->hasData() ? std::move(bucket) : nullptr; + + // Fill does not have layout properties to apply. + + addBucketGeometries(bucket, layer, styleBucket.filter); + + insertBucket(styleBucket.name, std::move(bucket)); } -std::unique_ptr TileWorker::createLineBucket(const GeometryTileLayer& layer, - const StyleBucket& bucket_desc) { +void TileWorker::createLineBucket(const GeometryTileLayer& layer, + const StyleBucket& styleBucket) { auto bucket = std::make_unique(); const float z = id.z; auto& layout = bucket->layout; - applyLayoutProperty(PropertyKey::LineCap, bucket_desc.layout, layout.cap, z); - applyLayoutProperty(PropertyKey::LineJoin, bucket_desc.layout, layout.join, z); - applyLayoutProperty(PropertyKey::LineMiterLimit, bucket_desc.layout, layout.miter_limit, z); - applyLayoutProperty(PropertyKey::LineRoundLimit, bucket_desc.layout, layout.round_limit, z); + applyLayoutProperty(PropertyKey::LineCap, styleBucket.layout, layout.cap, z); + applyLayoutProperty(PropertyKey::LineJoin, styleBucket.layout, layout.join, z); + applyLayoutProperty(PropertyKey::LineMiterLimit, styleBucket.layout, layout.miter_limit, z); + applyLayoutProperty(PropertyKey::LineRoundLimit, styleBucket.layout, layout.round_limit, z); + + addBucketGeometries(bucket, layer, styleBucket.filter); - addBucketGeometries(bucket, layer, bucket_desc.filter); - return bucket->hasData() ? std::move(bucket) : nullptr; + insertBucket(styleBucket.name, std::move(bucket)); } -std::unique_ptr TileWorker::createCircleBucket(const GeometryTileLayer& layer, - const StyleBucket& bucket_desc) { +void TileWorker::createCircleBucket(const GeometryTileLayer& layer, + const StyleBucket& styleBucket) { auto bucket = std::make_unique(); // Circle does not have layout properties to apply. - addBucketGeometries(bucket, layer, bucket_desc.filter); - return bucket->hasData() ? std::move(bucket) : nullptr; + addBucketGeometries(bucket, layer, styleBucket.filter); + + insertBucket(styleBucket.name, std::move(bucket)); } -std::unique_ptr TileWorker::createSymbolBucket(const GeometryTileLayer& layer, - const StyleBucket& bucket_desc) { +void TileWorker::createSymbolBucket(const GeometryTileLayer& layer, + const StyleBucket& styleBucket) { auto bucket = std::make_unique(id.overscaling, id.z); const float z = id.z; auto& layout = bucket->layout; - applyLayoutProperty(PropertyKey::SymbolPlacement, bucket_desc.layout, layout.placement, z); + applyLayoutProperty(PropertyKey::SymbolPlacement, styleBucket.layout, layout.placement, z); if (layout.placement == PlacementType::Line) { layout.icon.rotation_alignment = RotationAlignmentType::Map; layout.text.rotation_alignment = RotationAlignmentType::Map; }; - applyLayoutProperty(PropertyKey::SymbolSpacing, bucket_desc.layout, layout.spacing, z); - applyLayoutProperty(PropertyKey::SymbolAvoidEdges, bucket_desc.layout, layout.avoid_edges, z); - - applyLayoutProperty(PropertyKey::IconAllowOverlap, bucket_desc.layout, layout.icon.allow_overlap, z); - applyLayoutProperty(PropertyKey::IconIgnorePlacement, bucket_desc.layout, layout.icon.ignore_placement, z); - applyLayoutProperty(PropertyKey::IconOptional, bucket_desc.layout, layout.icon.optional, z); - applyLayoutProperty(PropertyKey::IconRotationAlignment, bucket_desc.layout, layout.icon.rotation_alignment, z); - applyLayoutProperty(PropertyKey::IconImage, bucket_desc.layout, layout.icon.image, z); - applyLayoutProperty(PropertyKey::IconPadding, bucket_desc.layout, layout.icon.padding, z); - applyLayoutProperty(PropertyKey::IconRotate, bucket_desc.layout, layout.icon.rotate, z); - applyLayoutProperty(PropertyKey::IconKeepUpright, bucket_desc.layout, layout.icon.keep_upright, z); - applyLayoutProperty(PropertyKey::IconOffset, bucket_desc.layout, layout.icon.offset, z); - - applyLayoutProperty(PropertyKey::TextRotationAlignment, bucket_desc.layout, layout.text.rotation_alignment, z); - applyLayoutProperty(PropertyKey::TextField, bucket_desc.layout, layout.text.field, z); - applyLayoutProperty(PropertyKey::TextFont, bucket_desc.layout, layout.text.font, z); - applyLayoutProperty(PropertyKey::TextMaxWidth, bucket_desc.layout, layout.text.max_width, z); - applyLayoutProperty(PropertyKey::TextLineHeight, bucket_desc.layout, layout.text.line_height, z); - applyLayoutProperty(PropertyKey::TextLetterSpacing, bucket_desc.layout, layout.text.letter_spacing, z); - applyLayoutProperty(PropertyKey::TextMaxAngle, bucket_desc.layout, layout.text.max_angle, z); - applyLayoutProperty(PropertyKey::TextRotate, bucket_desc.layout, layout.text.rotate, z); - applyLayoutProperty(PropertyKey::TextPadding, bucket_desc.layout, layout.text.padding, z); - applyLayoutProperty(PropertyKey::TextIgnorePlacement, bucket_desc.layout, layout.text.ignore_placement, z); - applyLayoutProperty(PropertyKey::TextOptional, bucket_desc.layout, layout.text.optional, z); - applyLayoutProperty(PropertyKey::TextJustify, bucket_desc.layout, layout.text.justify, z); - applyLayoutProperty(PropertyKey::TextAnchor, bucket_desc.layout, layout.text.anchor, z); - applyLayoutProperty(PropertyKey::TextKeepUpright, bucket_desc.layout, layout.text.keep_upright, z); - applyLayoutProperty(PropertyKey::TextTransform, bucket_desc.layout, layout.text.transform, z); - applyLayoutProperty(PropertyKey::TextOffset, bucket_desc.layout, layout.text.offset, z); - applyLayoutProperty(PropertyKey::TextAllowOverlap, bucket_desc.layout, layout.text.allow_overlap, z); - - applyLayoutProperty(PropertyKey::IconSize, bucket_desc.layout, layout.icon.size, z + 1); - applyLayoutProperty(PropertyKey::IconSize, bucket_desc.layout, layout.icon.max_size, 18); - applyLayoutProperty(PropertyKey::TextSize, bucket_desc.layout, layout.text.size, z + 1); - applyLayoutProperty(PropertyKey::TextSize, bucket_desc.layout, layout.text.max_size, 18); - - if (bucket->needsDependencies(layer, bucket_desc.filter, *style.glyphStore, *style.sprite)) { - partialParse = true; + applyLayoutProperty(PropertyKey::SymbolSpacing, styleBucket.layout, layout.spacing, z); + applyLayoutProperty(PropertyKey::SymbolAvoidEdges, styleBucket.layout, layout.avoid_edges, z); + + applyLayoutProperty(PropertyKey::IconAllowOverlap, styleBucket.layout, layout.icon.allow_overlap, z); + applyLayoutProperty(PropertyKey::IconIgnorePlacement, styleBucket.layout, layout.icon.ignore_placement, z); + applyLayoutProperty(PropertyKey::IconOptional, styleBucket.layout, layout.icon.optional, z); + applyLayoutProperty(PropertyKey::IconRotationAlignment, styleBucket.layout, layout.icon.rotation_alignment, z); + applyLayoutProperty(PropertyKey::IconImage, styleBucket.layout, layout.icon.image, z); + applyLayoutProperty(PropertyKey::IconPadding, styleBucket.layout, layout.icon.padding, z); + applyLayoutProperty(PropertyKey::IconRotate, styleBucket.layout, layout.icon.rotate, z); + applyLayoutProperty(PropertyKey::IconKeepUpright, styleBucket.layout, layout.icon.keep_upright, z); + applyLayoutProperty(PropertyKey::IconOffset, styleBucket.layout, layout.icon.offset, z); + + applyLayoutProperty(PropertyKey::TextRotationAlignment, styleBucket.layout, layout.text.rotation_alignment, z); + applyLayoutProperty(PropertyKey::TextField, styleBucket.layout, layout.text.field, z); + applyLayoutProperty(PropertyKey::TextFont, styleBucket.layout, layout.text.font, z); + applyLayoutProperty(PropertyKey::TextMaxWidth, styleBucket.layout, layout.text.max_width, z); + applyLayoutProperty(PropertyKey::TextLineHeight, styleBucket.layout, layout.text.line_height, z); + applyLayoutProperty(PropertyKey::TextLetterSpacing, styleBucket.layout, layout.text.letter_spacing, z); + applyLayoutProperty(PropertyKey::TextMaxAngle, styleBucket.layout, layout.text.max_angle, z); + applyLayoutProperty(PropertyKey::TextRotate, styleBucket.layout, layout.text.rotate, z); + applyLayoutProperty(PropertyKey::TextPadding, styleBucket.layout, layout.text.padding, z); + applyLayoutProperty(PropertyKey::TextIgnorePlacement, styleBucket.layout, layout.text.ignore_placement, z); + applyLayoutProperty(PropertyKey::TextOptional, styleBucket.layout, layout.text.optional, z); + applyLayoutProperty(PropertyKey::TextJustify, styleBucket.layout, layout.text.justify, z); + applyLayoutProperty(PropertyKey::TextAnchor, styleBucket.layout, layout.text.anchor, z); + applyLayoutProperty(PropertyKey::TextKeepUpright, styleBucket.layout, layout.text.keep_upright, z); + applyLayoutProperty(PropertyKey::TextTransform, styleBucket.layout, layout.text.transform, z); + applyLayoutProperty(PropertyKey::TextOffset, styleBucket.layout, layout.text.offset, z); + applyLayoutProperty(PropertyKey::TextAllowOverlap, styleBucket.layout, layout.text.allow_overlap, z); + + applyLayoutProperty(PropertyKey::IconSize, styleBucket.layout, layout.icon.size, z + 1); + applyLayoutProperty(PropertyKey::IconSize, styleBucket.layout, layout.icon.max_size, 18); + applyLayoutProperty(PropertyKey::TextSize, styleBucket.layout, layout.text.size, z + 1); + applyLayoutProperty(PropertyKey::TextSize, styleBucket.layout, layout.text.max_size, 18); + + bucket->parseFeatures(layer, styleBucket.filter); + + const bool needsDependencies = bucket->needsDependencies(*style.glyphStore, *style.sprite); + if (needsDependencies) { + // We cannot parse this bucket yet. Instead, we're saving it for later. + pending.emplace_back(styleBucket, std::move(bucket)); + } else { + bucket->addFeatures(reinterpret_cast(this), *style.spriteAtlas, + *style.glyphAtlas, *style.glyphStore, *collisionTile); + insertBucket(styleBucket.name, std::move(bucket)); } +} - // We do not proceed if the parser is in a "partial" state because - // the layer ordering needs to be respected when calculating text - // collisions. Although, at this point, we requested all the resources - // needed by this tile. - if (partialParse) { - return nullptr; +void TileWorker::insertBucket(const std::string& name, std::unique_ptr bucket) { + if (bucket->hasData()) { + result.buckets.emplace_back(name, std::move(bucket)); } - - bucket->addFeatures(reinterpret_cast(this), *style.spriteAtlas, *style.glyphAtlas, - *style.glyphStore, *collisionTile); - - return bucket->hasData() ? std::move(bucket) : nullptr; } diff --git a/src/mbgl/map/tile_worker.hpp b/src/mbgl/map/tile_worker.hpp index 689367c0521..f7cfa57ac6f 100644 --- a/src/mbgl/map/tile_worker.hpp +++ b/src/mbgl/map/tile_worker.hpp @@ -12,6 +12,7 @@ #include #include #include +#include #include namespace mbgl { @@ -24,9 +25,16 @@ class StyleLayer; class StyleBucket; class GeometryTileLayer; -using TileParseResult = mapbox::util::variant< - TileData::State, // success - std::string>; // error +// We're using this class to shuttle the resulting buckets from the worker thread to the MapContext +// thread. This class is movable-only because the vector contains movable-only value elements. +class TileParseResultBuckets { +public: + TileData::State state = TileData::State::invalid; + std::vector>> buckets; +}; + +using TileParseResult = mapbox::util::variant; // error class TileWorker : public util::noncopyable { public: @@ -38,20 +46,24 @@ class TileWorker : public util::noncopyable { std::unique_ptr); ~TileWorker(); - Bucket* getBucket(const StyleLayer&) const; - - TileParseResult parse(const GeometryTile&); - void redoPlacement(float angle, float pitch, bool collisionDebug); + TileParseResult parseAllLayers(const GeometryTile&); + TileParseResult parsePendingLayers(); + void redoPlacement(const std::unordered_map>*, + float angle, + float pitch, + bool collisionDebug); std::vector> layers; private: void parseLayer(const StyleLayer&, const GeometryTile&); - std::unique_ptr createFillBucket(const GeometryTileLayer&, const StyleBucket&); - std::unique_ptr createLineBucket(const GeometryTileLayer&, const StyleBucket&); - std::unique_ptr createCircleBucket(const GeometryTileLayer&, const StyleBucket&); - std::unique_ptr createSymbolBucket(const GeometryTileLayer&, const StyleBucket&); + void createFillBucket(const GeometryTileLayer&, const StyleBucket&); + void createLineBucket(const GeometryTileLayer&, const StyleBucket&); + void createCircleBucket(const GeometryTileLayer&, const StyleBucket&); + void createSymbolBucket(const GeometryTileLayer&, const StyleBucket&); + + void insertBucket(const std::string& name, std::unique_ptr); template void addBucketGeometries(Bucket&, const GeometryTileLayer&, const FilterExpression&); @@ -63,19 +75,16 @@ class TileWorker : public util::noncopyable { Style& style; const std::atomic& state; - bool partialParse = false; - std::unique_ptr collisionTile; - // Contains all the Bucket objects for the tile. Buckets are render - // objects and they get added to this map as they get processed. - // Tiles partially parsed can get new buckets at any moment but are - // also fit for rendering. That said, access to this list needs locking - // unless the tile is completely parsed. - std::unordered_map> buckets; - mutable std::mutex bucketsMutex; + // Contains buckets that we couldn't parse so far due to missing resources. + // They will be attempted on subsequent parses. + std::list>> pending; + + // Temporary holder + TileParseResultBuckets result; }; -} +} // namespace mbgl #endif diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp index 2e683daaffe..c3115cdb88b 100644 --- a/src/mbgl/map/vector_tile_data.cpp +++ b/src/mbgl/map/vector_tile_data.cpp @@ -43,11 +43,12 @@ void VectorTileData::request(float pixelRatio, const std::function& call FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Tile, url }, util::RunLoop::getLoop(), [url, callback, this](const Response &res) { - if (res.stale) { - // Only handle fresh responses. + // Do not cancel the request here; we want to get notified about tiles that expire. + + if (res.data && data == res.data) { + // We got the same data again. Abort early. return; } - req = nullptr; if (res.status == Response::NotFound) { state = State::parsed; @@ -57,39 +58,80 @@ void VectorTileData::request(float pixelRatio, const std::function& call if (res.status != Response::Successful) { std::stringstream message; - message << "Failed to load [" << url << "]: " << res.message; + message << "Failed to load [" << url << "]: " << res.message; error = message.str(); state = State::obsolete; callback(); return; } - state = State::loaded; + if (state == State::loading) { + state = State::loaded; + } else if (isReady()) { + state = State::partial; + } data = res.data; - reparse(callback); + parse(callback); }); } -bool VectorTileData::reparse(std::function callback) { - if (parsing || (state != State::loaded && state != State::partial)) { - return false; - } +void VectorTileData::parse(std::function callback) { + // Kick off a fresh parse of this tile. This happens when the tile is new, or + // when tile data changed. Replacing the workdRequest will cancel a pending work + // request in case there is one. + workRequest.reset(); + workRequest = worker.parseVectorTile(tileWorker, data, [this, callback] (TileParseResult result) { + workRequest.reset(); + if (state == State::obsolete) { + return; + } - parsing = true; + if (result.is()) { + auto& resultBuckets = result.get(); + state = resultBuckets.state; - workRequest = worker.parseVectorTile(tileWorker, data, [this, callback] (TileParseResult result) { - parsing = false; + // Move over all buckets we received in this parse request, potentially overwriting + // existing buckets in case we got a refresh parse. + for (auto& bucket : resultBuckets.buckets) { + buckets[bucket.first] = std::move(bucket.second); + } + } else { + std::stringstream message; + message << "Failed to parse [" << std::string(id) << "]: " << result.get(); + error = message.str(); + state = State::obsolete; + } + callback(); + }); +} + +bool VectorTileData::parsePending(std::function callback) { + if (workRequest) { + // There's already parsing or placement going on. + return false; + } + + workRequest.reset(); + workRequest = worker.parsePendingVectorTileLayers(tileWorker, [this, callback] (TileParseResult result) { + workRequest.reset(); if (state == State::obsolete) { return; } - if (result.is()) { - state = result.get(); + if (result.is()) { + auto& resultBuckets = result.get(); + state = resultBuckets.state; + + // Move over all buckets we received in this parse request, potentially overwriting + // existing buckets in case we got a refresh parse. + for (auto& bucket : resultBuckets.buckets) { + buckets[bucket.first] = std::move(bucket.second); + } } else { std::stringstream message; - message << "Failed to parse [" << std::string(id) << "]: " << result.get(); + message << "Failed to parse [" << std::string(id) << "]: " << result.get(); error = message.str(); state = State::obsolete; } @@ -101,39 +143,46 @@ bool VectorTileData::reparse(std::function callback) { } Bucket* VectorTileData::getBucket(const StyleLayer& layer) { - if (!isReady() || !layer.bucket) { + if (!layer.bucket) { return nullptr; } - return tileWorker.getBucket(layer); + const auto it = buckets.find(layer.bucket->name); + if (it == buckets.end()) { + return nullptr; + } + + assert(it->second); + return it->second.get(); } void VectorTileData::redoPlacement(float angle, float pitch, bool collisionDebug) { - if (angle == currentAngle && - pitch == currentPitch && - collisionDebug == currentCollisionDebug) + if (angle == currentAngle && pitch == currentPitch && collisionDebug == currentCollisionDebug) return; lastAngle = angle; lastPitch = pitch; lastCollisionDebug = collisionDebug; - if (state != State::parsed || redoingPlacement) + if (workRequest) { + // Don't start a new placement request when the current one hasn't completed yet, or when + // we are parsing buckets. return; + } - redoingPlacement = true; currentAngle = angle; currentPitch = pitch; currentCollisionDebug = collisionDebug; - workRequest = worker.redoPlacement(tileWorker, angle, pitch, collisionDebug, [this] { + workRequest.reset(); + workRequest = worker.redoPlacement(tileWorker, buckets, angle, pitch, collisionDebug, [this] { + workRequest.reset(); for (const auto& layer : tileWorker.layers) { auto bucket = getBucket(*layer); if (bucket) { bucket->swapRenderData(); } } - redoingPlacement = false; redoPlacement(lastAngle, lastPitch, lastCollisionDebug); }); } diff --git a/src/mbgl/map/vector_tile_data.hpp b/src/mbgl/map/vector_tile_data.hpp index 9b4fa8622bb..8497323e528 100644 --- a/src/mbgl/map/vector_tile_data.hpp +++ b/src/mbgl/map/vector_tile_data.hpp @@ -16,20 +16,16 @@ class Request; class VectorTileData : public TileData { public: - VectorTileData(const TileID&, - Style&, - const SourceInfo&, - float angle_, - float pitch_, - bool collisionDebug_); + VectorTileData( + const TileID&, Style&, const SourceInfo&, float angle_, float pitch_, bool collisionDebug_); ~VectorTileData(); Bucket* getBucket(const StyleLayer&) override; - void request(float pixelRatio, - const std::function& callback); + void request(float pixelRatio, const std::function& callback); - bool reparse(std::function callback) override; + void parse(std::function callback); + bool parsePending(std::function callback) override; void redoPlacement(float angle, float pitch, bool collisionDebug) override; @@ -39,7 +35,11 @@ class VectorTileData : public TileData { Worker& worker; TileWorker tileWorker; std::unique_ptr workRequest; - bool parsing = false; + + // Contains all the Bucket objects for the tile. Buckets are render + // objects and they get added by tile parsing operations. + std::unordered_map> buckets; + const SourceInfo& source; RequestHolder req; std::shared_ptr data; @@ -49,9 +49,8 @@ class VectorTileData : public TileData { float currentPitch; bool lastCollisionDebug = 0; bool currentCollisionDebug = 0; - bool redoingPlacement = false; }; -} +} // namespace mbgl #endif diff --git a/src/mbgl/renderer/bucket.hpp b/src/mbgl/renderer/bucket.hpp index a1dbdeeed72..9e89c881354 100644 --- a/src/mbgl/renderer/bucket.hpp +++ b/src/mbgl/renderer/bucket.hpp @@ -32,6 +32,8 @@ class Bucket : private util::noncopyable { virtual ~Bucket() {} + virtual bool hasData() const = 0; + inline bool needsUpload() const { return !uploaded; } diff --git a/src/mbgl/renderer/circle_bucket.hpp b/src/mbgl/renderer/circle_bucket.hpp index a83d158cb2b..0b9b4834d0e 100644 --- a/src/mbgl/renderer/circle_bucket.hpp +++ b/src/mbgl/renderer/circle_bucket.hpp @@ -26,7 +26,7 @@ class CircleBucket : public Bucket { void upload() override; void render(Painter&, const StyleLayer&, const TileID&, const mat4&) override; - bool hasData() const; + bool hasData() const override; void addGeometry(const GeometryCollection&); void drawCircles(CircleShader& shader); diff --git a/src/mbgl/renderer/debug_bucket.cpp b/src/mbgl/renderer/debug_bucket.cpp index 161412f0dc9..63562a67149 100644 --- a/src/mbgl/renderer/debug_bucket.cpp +++ b/src/mbgl/renderer/debug_bucket.cpp @@ -5,21 +5,13 @@ #include #include +#include using namespace mbgl; -DebugBucket::DebugBucket(DebugFontBuffer& fontBuffer_) - : fontBuffer(fontBuffer_) { -} - -void DebugBucket::upload() { - fontBuffer.upload(); - - uploaded = true; -} - -void DebugBucket::render(Painter& painter, const StyleLayer&, const TileID&, const mat4& matrix) { - painter.renderDebugText(*this, matrix); +DebugBucket::DebugBucket(const TileID id, const TileData::State state_) : state(state_) { + const std::string text = std::string(id) + " - " + TileData::StateToString(state); + fontBuffer.addText(text.c_str(), 50, 200, 5); } void DebugBucket::drawLines(PlainShader& shader) { diff --git a/src/mbgl/renderer/debug_bucket.hpp b/src/mbgl/renderer/debug_bucket.hpp index e3c01bbddbe..a5b163c0a30 100644 --- a/src/mbgl/renderer/debug_bucket.hpp +++ b/src/mbgl/renderer/debug_bucket.hpp @@ -1,28 +1,25 @@ #ifndef MBGL_RENDERER_DEBUGBUCKET #define MBGL_RENDERER_DEBUGBUCKET -#include +#include #include #include -#include - namespace mbgl { class PlainShader; -class DebugBucket : public Bucket { +class DebugBucket : private util::noncopyable { public: - DebugBucket(DebugFontBuffer& fontBuffer); - - void upload() override; - void render(Painter&, const StyleLayer&, const TileID&, const mat4&) override; + DebugBucket(TileID id, TileData::State); void drawLines(PlainShader& shader); void drawPoints(PlainShader& shader); + const TileData::State state; + private: - DebugFontBuffer& fontBuffer; + DebugFontBuffer fontBuffer; VertexArrayObject array; }; diff --git a/src/mbgl/renderer/fill_bucket.hpp b/src/mbgl/renderer/fill_bucket.hpp index 10ee65fd01f..054194340b8 100644 --- a/src/mbgl/renderer/fill_bucket.hpp +++ b/src/mbgl/renderer/fill_bucket.hpp @@ -34,7 +34,7 @@ class FillBucket : public Bucket { void upload() override; void render(Painter&, const StyleLayer&, const TileID&, const mat4&) override; - bool hasData() const; + bool hasData() const override; void addGeometry(const GeometryCollection&); void tessellate(); diff --git a/src/mbgl/renderer/line_bucket.hpp b/src/mbgl/renderer/line_bucket.hpp index 2e220829b0f..d890493d0ec 100644 --- a/src/mbgl/renderer/line_bucket.hpp +++ b/src/mbgl/renderer/line_bucket.hpp @@ -30,7 +30,7 @@ class LineBucket : public Bucket { void upload() override; void render(Painter&, const StyleLayer&, const TileID&, const mat4&) override; - bool hasData() const; + bool hasData() const override; void addGeometry(const GeometryCollection&); void addGeometry(const std::vector& line); diff --git a/src/mbgl/renderer/painter.hpp b/src/mbgl/renderer/painter.hpp index b7402d69b63..c093f38acd0 100644 --- a/src/mbgl/renderer/painter.hpp +++ b/src/mbgl/renderer/painter.hpp @@ -102,7 +102,7 @@ class Painter : private util::noncopyable { // Renders the red debug frame around a tile, visualizing its perimeter. void renderDebugFrame(const mat4 &matrix); - void renderDebugText(DebugBucket&, const mat4&); + void renderDebugText(TileData&, const mat4&); void renderFill(FillBucket&, const FillLayer&, const TileID&, const mat4&); void renderLine(LineBucket&, const LineLayer&, const TileID&, const mat4&); void renderCircle(CircleBucket&, const CircleLayer&, const TileID&, const mat4&); diff --git a/src/mbgl/renderer/painter_debug.cpp b/src/mbgl/renderer/painter_debug.cpp index c401faaaa25..5c6573aa7f3 100644 --- a/src/mbgl/renderer/painter_debug.cpp +++ b/src/mbgl/renderer/painter_debug.cpp @@ -14,34 +14,38 @@ void Painter::renderTileDebug(const Tile& tile) { assert(tile.data); if (data.getDebug()) { prepareTile(tile); - renderDebugText(tile.data->debugBucket, tile.matrix); + renderDebugText(*tile.data, tile.matrix); renderDebugFrame(tile.matrix); } } -void Painter::renderDebugText(DebugBucket& bucket, const mat4 &matrix) { +void Painter::renderDebugText(TileData& tileData, const mat4 &matrix) { MBGL_DEBUG_GROUP("debug text"); config.depthTest = false; + if (!tileData.debugBucket || tileData.debugBucket->state != tileData.getState()) { + tileData.debugBucket = std::make_unique(tileData.id, tileData.getState()); + } + useProgram(plainShader->program); plainShader->u_matrix = matrix; // Draw white outline plainShader->u_color = {{ 1.0f, 1.0f, 1.0f, 1.0f }}; lineWidth(4.0f * data.pixelRatio); - bucket.drawLines(*plainShader); + tileData.debugBucket->drawLines(*plainShader); #ifndef GL_ES_VERSION_2_0 // Draw line "end caps" MBGL_CHECK_ERROR(glPointSize(2)); - bucket.drawPoints(*plainShader); + tileData.debugBucket->drawPoints(*plainShader); #endif // Draw black text. plainShader->u_color = {{ 0.0f, 0.0f, 0.0f, 1.0f }}; lineWidth(2.0f * data.pixelRatio); - bucket.drawLines(*plainShader); + tileData.debugBucket->drawLines(*plainShader); config.depthTest = true; } diff --git a/src/mbgl/renderer/raster_bucket.hpp b/src/mbgl/renderer/raster_bucket.hpp index a20828102d8..91e2b3398d1 100644 --- a/src/mbgl/renderer/raster_bucket.hpp +++ b/src/mbgl/renderer/raster_bucket.hpp @@ -18,7 +18,7 @@ class RasterBucket : public Bucket { void upload() override; void render(Painter&, const StyleLayer&, const TileID&, const mat4&) override; - bool hasData() const; + bool hasData() const override; bool setImage(std::unique_ptr image); diff --git a/src/mbgl/renderer/symbol_bucket.cpp b/src/mbgl/renderer/symbol_bucket.cpp index d769060c6e9..d85dbaf8587 100644 --- a/src/mbgl/renderer/symbol_bucket.cpp +++ b/src/mbgl/renderer/symbol_bucket.cpp @@ -90,20 +90,16 @@ bool SymbolBucket::hasIconData() const { return renderData && !renderData->icon. bool SymbolBucket::hasCollisionBoxData() const { return renderData && !renderData->collisionBox.groups.empty(); } -bool SymbolBucket::needsDependencies(const GeometryTileLayer& layer, - const FilterExpression& filter, - GlyphStore& glyphStore, - Sprite& sprite) { +void SymbolBucket::parseFeatures(const GeometryTileLayer& layer, + const FilterExpression& filter) { const bool has_text = !layout.text.field.empty() && !layout.text.font.empty(); const bool has_icon = !layout.icon.image.empty(); if (!has_text && !has_icon) { - return false; + return; } // Determine and load glyph ranges - std::set ranges; - const GLsizei featureCount = static_cast(layer.featureCount()); for (GLsizei i = 0; i < featureCount; i++) { auto feature = layer.getFeature(i); @@ -161,12 +157,15 @@ bool SymbolBucket::needsDependencies(const GeometryTileLayer& layer, if (layout.placement == PlacementType::Line) { util::mergeLines(features); } +} - if (!glyphStore.hasGlyphRanges(layout.text.font, ranges)) { +bool SymbolBucket::needsDependencies(GlyphStore& glyphStore, + Sprite& sprite) { + if (!layout.text.field.empty() && !layout.text.font.empty() && !glyphStore.hasGlyphRanges(layout.text.font, ranges)) { return true; } - if (!sprite.isLoaded()) { + if (!layout.icon.image.empty() && !sprite.isLoaded()) { return true; } diff --git a/src/mbgl/renderer/symbol_bucket.hpp b/src/mbgl/renderer/symbol_bucket.hpp index c3c39627cfa..54015ae1b99 100644 --- a/src/mbgl/renderer/symbol_bucket.hpp +++ b/src/mbgl/renderer/symbol_bucket.hpp @@ -17,6 +17,7 @@ #include #include +#include #include namespace mbgl { @@ -69,7 +70,7 @@ class SymbolBucket : public Bucket { void upload() override; void render(Painter&, const StyleLayer&, const TileID&, const mat4&) override; - bool hasData() const; + bool hasData() const override; bool hasTextData() const; bool hasIconData() const; bool hasCollisionBoxData() const; @@ -85,10 +86,10 @@ class SymbolBucket : public Bucket { void drawIcons(IconShader& shader); void drawCollisionBoxes(CollisionBoxShader& shader); - bool needsDependencies(const GeometryTileLayer&, - const FilterExpression&, - GlyphStore&, - Sprite&); + void parseFeatures(const GeometryTileLayer&, + const FilterExpression&); + bool needsDependencies(GlyphStore& glyphStore, + Sprite& sprite); void placeFeatures(CollisionTile&) override; private: @@ -120,6 +121,7 @@ class SymbolBucket : public Bucket { const float tileExtent = 4096.0f; const float tilePixelRatio; + std::set ranges; std::vector symbolInstances; std::vector features; diff --git a/src/mbgl/util/worker.cpp b/src/mbgl/util/worker.cpp index fc599713b3d..371766f0961 100644 --- a/src/mbgl/util/worker.cpp +++ b/src/mbgl/util/worker.cpp @@ -16,7 +16,9 @@ class Worker::Impl { public: Impl() = default; - void parseRasterTile(RasterBucket* bucket, const std::shared_ptr data, std::function callback) { + void parseRasterTile(std::unique_ptr bucket, + const std::shared_ptr data, + std::function callback) { std::unique_ptr image(new util::Image(*data)); if (!(*image)) { callback(TileParseResult("error parsing raster image")); @@ -26,34 +28,56 @@ class Worker::Impl { callback(TileParseResult("error setting raster image to bucket")); } - callback(TileParseResult(TileData::State::parsed)); + TileParseResultBuckets result; + result.buckets.emplace_back("raster", std::move(bucket)); + result.state = TileData::State::parsed; + + callback(std::move(result)); } - void parseVectorTile(TileWorker* worker, const std::shared_ptr data, std::function callback) { + void parseVectorTile(TileWorker* worker, + const std::shared_ptr data, + std::function callback) { try { pbf tilePBF(reinterpret_cast(data->data()), data->size()); - callback(worker->parse(VectorTile(tilePBF))); + callback(worker->parseAllLayers(VectorTile(tilePBF))); + } catch (const std::exception& ex) { + callback(TileParseResult(ex.what())); + } + } + + void parsePendingVectorTileLayers(TileWorker* worker, + std::function callback) { + try { + callback(worker->parsePendingLayers()); } catch (const std::exception& ex) { callback(TileParseResult(ex.what())); } } - void parseLiveTile(TileWorker* worker, const AnnotationTile* tile, std::function callback) { + void parseLiveTile(TileWorker* worker, + const AnnotationTile* tile, + std::function callback) { try { - callback(worker->parse(*tile)); + callback(worker->parseAllLayers(*tile)); } catch (const std::exception& ex) { callback(TileParseResult(ex.what())); } } - void redoPlacement(TileWorker* worker, float angle, float pitch, bool collisionDebug, std::function callback) { - worker->redoPlacement(angle, pitch, collisionDebug); + void redoPlacement(TileWorker* worker, + const std::unordered_map>* buckets, + float angle, + float pitch, + bool collisionDebug, + std::function callback) { + worker->redoPlacement(buckets, angle, pitch, collisionDebug); callback(); } }; Worker::Worker(std::size_t count) { - util::ThreadContext context = {"Worker", util::ThreadType::Worker, util::ThreadPriority::Low}; + util::ThreadContext context = { "Worker", util::ThreadType::Worker, util::ThreadPriority::Low }; for (std::size_t i = 0; i < count; i++) { threads.emplace_back(std::make_unique>(context)); } @@ -61,24 +85,50 @@ Worker::Worker(std::size_t count) { Worker::~Worker() = default; -std::unique_ptr Worker::parseRasterTile(RasterBucket& bucket, const std::shared_ptr data, std::function callback) { +std::unique_ptr +Worker::parseRasterTile(std::unique_ptr bucket, + const std::shared_ptr data, + std::function callback) { + current = (current + 1) % threads.size(); + return threads[current]->invokeWithCallback(&Worker::Impl::parseRasterTile, callback, bucket, + data); +} + +std::unique_ptr +Worker::parseVectorTile(TileWorker& worker, + const std::shared_ptr data, + std::function callback) { current = (current + 1) % threads.size(); - return threads[current]->invokeWithCallback(&Worker::Impl::parseRasterTile, callback, &bucket, data); + return threads[current]->invokeWithCallback(&Worker::Impl::parseVectorTile, callback, &worker, + data); } -std::unique_ptr Worker::parseVectorTile(TileWorker& worker, const std::shared_ptr data, std::function callback) { +std::unique_ptr +Worker::parsePendingVectorTileLayers(TileWorker& worker, + std::function callback) { current = (current + 1) % threads.size(); - return threads[current]->invokeWithCallback(&Worker::Impl::parseVectorTile, callback, &worker, data); + return threads[current]->invokeWithCallback(&Worker::Impl::parsePendingVectorTileLayers, + callback, &worker); } -std::unique_ptr Worker::parseLiveTile(TileWorker& worker, const AnnotationTile& tile, std::function callback) { +std::unique_ptr Worker::parseLiveTile(TileWorker& worker, + const AnnotationTile& tile, + std::function callback) { current = (current + 1) % threads.size(); - return threads[current]->invokeWithCallback(&Worker::Impl::parseLiveTile, callback, &worker, &tile); + return threads[current]->invokeWithCallback(&Worker::Impl::parseLiveTile, callback, &worker, + &tile); } -std::unique_ptr Worker::redoPlacement(TileWorker& worker, float angle, float pitch, bool collisionDebug, std::function callback) { +std::unique_ptr +Worker::redoPlacement(TileWorker& worker, + const std::unordered_map>& buckets, + float angle, + float pitch, + bool collisionDebug, + std::function callback) { current = (current + 1) % threads.size(); - return threads[current]->invokeWithCallback(&Worker::Impl::redoPlacement, callback, &worker, angle, pitch, collisionDebug); + return threads[current]->invokeWithCallback(&Worker::Impl::redoPlacement, callback, &worker, + &buckets, angle, pitch, collisionDebug); } } // end namespace mbgl diff --git a/src/mbgl/util/worker.hpp b/src/mbgl/util/worker.hpp index 18b1bc92b1a..7a92a09a514 100644 --- a/src/mbgl/util/worker.hpp +++ b/src/mbgl/util/worker.hpp @@ -31,34 +31,33 @@ class Worker : public mbgl::util::noncopyable { using Request = std::unique_ptr; - Request parseRasterTile( - RasterBucket&, - std::shared_ptr data, - std::function callback); + Request parseRasterTile(std::unique_ptr bucket, + std::shared_ptr data, + std::function callback); - Request parseVectorTile( - TileWorker&, - std::shared_ptr data, - std::function callback); + Request parseVectorTile(TileWorker&, + std::shared_ptr data, + std::function callback); - Request parseLiveTile( - TileWorker&, - const AnnotationTile&, - std::function callback); + Request parsePendingVectorTileLayers(TileWorker&, + std::function callback); - Request redoPlacement( - TileWorker&, - float angle, - float pitch, - bool collisionDebug, - std::function callback); + Request parseLiveTile(TileWorker&, + const AnnotationTile&, + std::function callback); + + Request redoPlacement(TileWorker&, + const std::unordered_map>&, + float angle, + float pitch, + bool collisionDebug, + std::function callback); private: class Impl; std::vector>> threads; std::size_t current = 0; }; - } #endif diff --git a/test/style/resource_loading.cpp b/test/style/resource_loading.cpp index e0c4590c7c3..c6f1c1dfaec 100644 --- a/test/style/resource_loading.cpp +++ b/test/style/resource_loading.cpp @@ -38,6 +38,10 @@ class MockMapContext : public Style::Observer { } ~MockMapContext() { + cleanup(); + } + + void cleanup() { style_.reset(); } @@ -118,6 +122,7 @@ void runTestCase(MockFileSource::Type type, // Needed because it will make the Map thread // join and cease logging after this point. + context->invoke(&MockMapContext::cleanup); context.reset(); uint32_t match = 0; From 75dec6ffac6f3e79e5a173cd8a3f98d374ed1c09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Mon, 26 Oct 2015 15:45:32 +0100 Subject: [PATCH 8/8] [core] always reparse with the freshest possible placement config Fixes an issue where updates to stale tiles would remove labels altogether until the map was rotated. --- platform/default/glfw_view.cpp | 2 +- src/mbgl/map/live_tile_data.cpp | 64 +++++++++++++++++++---- src/mbgl/map/live_tile_data.hpp | 10 ++++ src/mbgl/map/source.cpp | 10 ++-- src/mbgl/map/tile_data.hpp | 3 +- src/mbgl/map/tile_worker.cpp | 24 +++++---- src/mbgl/map/tile_worker.hpp | 10 ++-- src/mbgl/map/vector_tile_data.cpp | 78 ++++++++++++++++------------- src/mbgl/map/vector_tile_data.hpp | 19 ++++--- src/mbgl/renderer/symbol_bucket.cpp | 24 +++++---- src/mbgl/text/collision_tile.cpp | 13 +++-- src/mbgl/text/collision_tile.hpp | 38 +++++++------- src/mbgl/text/placement_config.hpp | 28 +++++++++++ src/mbgl/util/worker.cpp | 24 ++++----- src/mbgl/util/worker.hpp | 6 +-- 15 files changed, 227 insertions(+), 126 deletions(-) create mode 100644 src/mbgl/text/placement_config.hpp diff --git a/platform/default/glfw_view.cpp b/platform/default/glfw_view.cpp index cbc872313e9..0085e036b1c 100644 --- a/platform/default/glfw_view.cpp +++ b/platform/default/glfw_view.cpp @@ -213,7 +213,7 @@ void GLFWView::addRandomPointAnnotations(int count) { std::vector points; for (int i = 0; i < count; i++) { - points.emplace_back(makeRandomPoint(), "default_marker"); + points.emplace_back(makeRandomPoint(), "marker-15"); } auto newIDs = map->addPointAnnotations(points); diff --git a/src/mbgl/map/live_tile_data.cpp b/src/mbgl/map/live_tile_data.cpp index fb8e6c3605a..89b1d6fda45 100644 --- a/src/mbgl/map/live_tile_data.cpp +++ b/src/mbgl/map/live_tile_data.cpp @@ -14,17 +14,12 @@ using namespace mbgl; LiveTileData::LiveTileData(const TileID& id_, std::unique_ptr tile_, - Style& style_, - const SourceInfo& source_, + Style& style, + const SourceInfo& source, std::function callback) : TileData(id_), - worker(style_.workers), - tileWorker(id_, - source_.source_id, - style_, - style_.layers, - state, - std::make_unique(0, 0, false)), + worker(style.workers), + tileWorker(id, source.source_id, style, style.layers, state), tile(std::move(tile_)) { state = State::loaded; @@ -41,25 +36,41 @@ bool LiveTileData::parsePending(std::function callback) { return false; } - workRequest = worker.parseLiveTile(tileWorker, *tile, [this, callback] (TileParseResult result) { + workRequest.reset(); + workRequest = worker.parseLiveTile(tileWorker, *tile, targetConfig, [this, callback, config = targetConfig] (TileParseResult result) { workRequest.reset(); if (result.is()) { auto& resultBuckets = result.get(); state = resultBuckets.state; + // Persist the configuration we just placed so that we can later check whether we need + // to place again in case the configuration has changed. + placedConfig = config; + // Move over all buckets we received in this parse request, potentially overwriting // existing buckets in case we got a refresh parse. for (auto& bucket : resultBuckets.buckets) { buckets[bucket.first] = std::move(bucket.second); } + // The target configuration could have changed since we started placement. In this case, + // we're starting another placement run. + if (placedConfig != targetConfig) { + redoPlacement(); + } } else { error = result.get(); state = State::obsolete; } callback(); + + // The target configuration could have changed since we started placement. In this case, + // we're starting another placement run. + if (!workRequest && placedConfig != targetConfig) { + redoPlacement(); + } }); return true; @@ -87,3 +98,36 @@ void LiveTileData::cancel() { state = State::obsolete; workRequest.reset(); } + +void LiveTileData::redoPlacement(const PlacementConfig newConfig) { + if (newConfig != placedConfig) { + targetConfig = newConfig; + + if (!workRequest) { + // Don't start a new placement request when the current one hasn't completed yet, or + // when we are parsing buckets. + redoPlacement(); + } + } +} + +void LiveTileData::redoPlacement() { + workRequest.reset(); + workRequest = worker.redoPlacement(tileWorker, buckets, targetConfig, [this, config = targetConfig] { + workRequest.reset(); + + // Persist the configuration we just placed so that we can later check whether we need to + // place again in case the configuration has changed. + placedConfig = config; + + for (auto& bucket : buckets) { + bucket.second->swapRenderData(); + } + + // The target configuration could have changed since we started placement. In this case, + // we're starting another placement run. + if (placedConfig != targetConfig) { + redoPlacement(); + } + }); +} diff --git a/src/mbgl/map/live_tile_data.hpp b/src/mbgl/map/live_tile_data.hpp index 568892216c4..958f1db50ab 100644 --- a/src/mbgl/map/live_tile_data.hpp +++ b/src/mbgl/map/live_tile_data.hpp @@ -22,6 +22,9 @@ class LiveTileData : public TileData { bool parsePending(std::function callback) override; + void redoPlacement(PlacementConfig config) override; + void redoPlacement(); + void cancel() override; Bucket* getBucket(const StyleLayer&) override; @@ -34,6 +37,13 @@ class LiveTileData : public TileData { // Contains all the Bucket objects for the tile. Buckets are render // objects and they get added by tile parsing operations. std::unordered_map> buckets; + + // Stores the placement configuration of the text that is currently placed on the screen. + PlacementConfig placedConfig; + + // Stores the placement configuration of how the text should be placed. This isn't necessarily + // the one that is being displayed. + PlacementConfig targetConfig; }; } diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index c79ef4b3b68..8a7f97d9633 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -286,8 +286,7 @@ TileData::State Source::addTile(MapData& data, // If we don't find working tile data, we're just going to load it. if (info.type == SourceType::Vector) { - auto tileData = std::make_shared(normalized_id, style, info, - transformState.getAngle(), transformState.getPitch(), data.getCollisionDebug()); + auto tileData = std::make_shared(normalized_id, style, info); tileData->request(data.pixelRatio, callback); new_tile.data = tileData; } else if (info.type == SourceType::Raster) { @@ -296,7 +295,7 @@ TileData::State Source::addTile(MapData& data, new_tile.data = tileData; } else if (info.type == SourceType::Annotations) { new_tile.data = std::make_shared(normalized_id, - data.getAnnotationManager()->getTile(normalized_id), style, info, callback); + data.getAnnotationManager()->getTile(normalized_id), style, info, callback); } else { throw std::runtime_error("source type not implemented"); } @@ -510,7 +509,8 @@ bool Source::update(MapData& data, updateTilePtrs(); for (auto& tilePtr : tilePtrs) { - tilePtr->data->redoPlacement(transformState.getAngle(), transformState.getPitch(), data.getCollisionDebug()); + tilePtr->data->redoPlacement( + { transformState.getAngle(), transformState.getPitch(), data.getCollisionDebug() }); } updated = data.getAnimationTime(); @@ -560,7 +560,7 @@ void Source::tileLoadingCompleteCallback(const TileID& normalized_id, const Tran return; } - data->redoPlacement(transformState.getAngle(), transformState.getPitch(), collisionDebug); + data->redoPlacement({ transformState.getAngle(), transformState.getPitch(), collisionDebug }); emitTileLoaded(true); } diff --git a/src/mbgl/map/tile_data.hpp b/src/mbgl/map/tile_data.hpp index 44f0304c8d9..d9895ce01a7 100644 --- a/src/mbgl/map/tile_data.hpp +++ b/src/mbgl/map/tile_data.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -76,7 +77,7 @@ class TileData : private util::noncopyable { virtual Bucket* getBucket(const StyleLayer&) = 0; virtual bool parsePending(std::function) { return true; } - virtual void redoPlacement(float, float, bool) {} + virtual void redoPlacement(PlacementConfig) {} bool isReady() const { return isReadyState(state); diff --git a/src/mbgl/map/tile_worker.cpp b/src/mbgl/map/tile_worker.cpp index c7836c4ba94..88bbf16a3f3 100644 --- a/src/mbgl/map/tile_worker.cpp +++ b/src/mbgl/map/tile_worker.cpp @@ -19,15 +19,13 @@ TileWorker::TileWorker(TileID id_, std::string sourceID_, Style& style_, std::vector> layers_, - const std::atomic& state_, - std::unique_ptr collision_) + const std::atomic& state_) : layers(std::move(layers_)), id(id_), sourceID(sourceID_), parameters(id.z), style(style_), - state(state_), - collisionTile(std::move(collision_)) { + state(state_) { assert(style.sprite); } @@ -35,10 +33,14 @@ TileWorker::~TileWorker() { style.glyphAtlas->removeGlyphs(reinterpret_cast(this)); } -TileParseResult TileWorker::parseAllLayers(const GeometryTile& geometryTile) { +TileParseResult TileWorker::parseAllLayers(const GeometryTile& geometryTile, + PlacementConfig config) { // We're doing a fresh parse of the tile, because the underlying data has changed. pending.clear(); + // Reset the collision tile so we have a clean slate; we're placing all features anyway. + collisionTile = std::make_unique(config); + // We're storing a list of buckets we've parsed to avoid parsing a bucket twice that is // referenced from more than one layer std::set parsed; @@ -84,12 +86,10 @@ TileParseResult TileWorker::parsePendingLayers() { void TileWorker::redoPlacement( const std::unordered_map>* buckets, - float angle, - float pitch, - bool collisionDebug) { + PlacementConfig config) { // Reset the collision tile so we have a clean slate; we're placing all features anyway. - collisionTile = std::make_unique(angle, pitch, collisionDebug); + collisionTile = std::make_unique(config); for (auto i = layers.rbegin(); i != layers.rend(); i++) { const auto it = buckets->find((*i)->id); @@ -268,11 +268,17 @@ void TileWorker::createSymbolBucket(const GeometryTileLayer& layer, bucket->parseFeatures(layer, styleBucket.filter); + assert(style.glyphStore); + assert(style.sprite); const bool needsDependencies = bucket->needsDependencies(*style.glyphStore, *style.sprite); if (needsDependencies) { // We cannot parse this bucket yet. Instead, we're saving it for later. pending.emplace_back(styleBucket, std::move(bucket)); } else { + assert(style.spriteAtlas); + assert(style.glyphAtlas); + assert(style.glyphStore); + assert(collisionTile); bucket->addFeatures(reinterpret_cast(this), *style.spriteAtlas, *style.glyphAtlas, *style.glyphStore, *collisionTile); insertBucket(styleBucket.name, std::move(bucket)); diff --git a/src/mbgl/map/tile_worker.hpp b/src/mbgl/map/tile_worker.hpp index f7cfa57ac6f..27fc0d37fea 100644 --- a/src/mbgl/map/tile_worker.hpp +++ b/src/mbgl/map/tile_worker.hpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -42,16 +43,13 @@ class TileWorker : public util::noncopyable { std::string sourceID, Style&, std::vector>, - const std::atomic&, - std::unique_ptr); + const std::atomic&); ~TileWorker(); - TileParseResult parseAllLayers(const GeometryTile&); + TileParseResult parseAllLayers(const GeometryTile&, PlacementConfig); TileParseResult parsePendingLayers(); void redoPlacement(const std::unordered_map>*, - float angle, - float pitch, - bool collisionDebug); + PlacementConfig); std::vector> layers; diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp index c3115cdb88b..3c831250c5c 100644 --- a/src/mbgl/map/vector_tile_data.cpp +++ b/src/mbgl/map/vector_tile_data.cpp @@ -14,23 +14,15 @@ using namespace mbgl; VectorTileData::VectorTileData(const TileID& id_, Style& style_, - const SourceInfo& source_, - float angle, - float pitch, - bool collisionDebug) + const SourceInfo& source_) : TileData(id_), worker(style_.workers), tileWorker(id_, source_.source_id, style_, style_.layers, - state, - std::make_unique(angle, pitch, collisionDebug)), - source(source_), - lastAngle(angle), - currentAngle(angle), - currentPitch(pitch), - currentCollisionDebug(collisionDebug) { + state), + source(source_) { } VectorTileData::~VectorTileData() { @@ -81,7 +73,7 @@ void VectorTileData::parse(std::function callback) { // when tile data changed. Replacing the workdRequest will cancel a pending work // request in case there is one. workRequest.reset(); - workRequest = worker.parseVectorTile(tileWorker, data, [this, callback] (TileParseResult result) { + workRequest = worker.parseVectorTile(tileWorker, data, targetConfig, [this, callback, config = targetConfig] (TileParseResult result) { workRequest.reset(); if (state == State::obsolete) { return; @@ -91,11 +83,21 @@ void VectorTileData::parse(std::function callback) { auto& resultBuckets = result.get(); state = resultBuckets.state; + // Persist the configuration we just placed so that we can later check whether we need to + // place again in case the configuration has changed. + placedConfig = config; + // Move over all buckets we received in this parse request, potentially overwriting // existing buckets in case we got a refresh parse. for (auto& bucket : resultBuckets.buckets) { buckets[bucket.first] = std::move(bucket.second); } + + // The target configuration could have changed since we started placement. In this case, + // we're starting another placement run. + if (placedConfig != targetConfig) { + redoPlacement(); + } } else { std::stringstream message; message << "Failed to parse [" << std::string(id) << "]: " << result.get(); @@ -129,6 +131,12 @@ bool VectorTileData::parsePending(std::function callback) { for (auto& bucket : resultBuckets.buckets) { buckets[bucket.first] = std::move(bucket.second); } + + // The target configuration could have changed since we started placement. In this case, + // we're starting another placement run. + if (placedConfig != targetConfig) { + redoPlacement(); + } } else { std::stringstream message; message << "Failed to parse [" << std::string(id) << "]: " << result.get(); @@ -156,34 +164,36 @@ Bucket* VectorTileData::getBucket(const StyleLayer& layer) { return it->second.get(); } -void VectorTileData::redoPlacement(float angle, float pitch, bool collisionDebug) { - if (angle == currentAngle && pitch == currentPitch && collisionDebug == currentCollisionDebug) - return; - - lastAngle = angle; - lastPitch = pitch; - lastCollisionDebug = collisionDebug; +void VectorTileData::redoPlacement(const PlacementConfig newConfig) { + if (newConfig != placedConfig) { + targetConfig = newConfig; - if (workRequest) { - // Don't start a new placement request when the current one hasn't completed yet, or when - // we are parsing buckets. - return; + if (!workRequest) { + // Don't start a new placement request when the current one hasn't completed yet, or when + // we are parsing buckets. + redoPlacement(); + } } +} - currentAngle = angle; - currentPitch = pitch; - currentCollisionDebug = collisionDebug; - +void VectorTileData::redoPlacement() { workRequest.reset(); - workRequest = worker.redoPlacement(tileWorker, buckets, angle, pitch, collisionDebug, [this] { + workRequest = worker.redoPlacement(tileWorker, buckets, targetConfig, [this, config = targetConfig] { workRequest.reset(); - for (const auto& layer : tileWorker.layers) { - auto bucket = getBucket(*layer); - if (bucket) { - bucket->swapRenderData(); - } + + // Persist the configuration we just placed so that we can later check whether we need to + // place again in case the configuration has changed. + placedConfig = config; + + for (auto& bucket : buckets) { + bucket.second->swapRenderData(); + } + + // The target configuration could have changed since we started placement. In this case, + // we're starting another placement run. + if (placedConfig != targetConfig) { + redoPlacement(); } - redoPlacement(lastAngle, lastPitch, lastCollisionDebug); }); } diff --git a/src/mbgl/map/vector_tile_data.hpp b/src/mbgl/map/vector_tile_data.hpp index 8497323e528..9faffab83f7 100644 --- a/src/mbgl/map/vector_tile_data.hpp +++ b/src/mbgl/map/vector_tile_data.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include @@ -17,7 +18,7 @@ class Request; class VectorTileData : public TileData { public: VectorTileData( - const TileID&, Style&, const SourceInfo&, float angle_, float pitch_, bool collisionDebug_); + const TileID&, Style&, const SourceInfo&); ~VectorTileData(); Bucket* getBucket(const StyleLayer&) override; @@ -27,7 +28,8 @@ class VectorTileData : public TileData { void parse(std::function callback); bool parsePending(std::function callback) override; - void redoPlacement(float angle, float pitch, bool collisionDebug) override; + void redoPlacement(PlacementConfig config) override; + void redoPlacement(); void cancel() override; @@ -43,12 +45,13 @@ class VectorTileData : public TileData { const SourceInfo& source; RequestHolder req; std::shared_ptr data; - float lastAngle = 0; - float currentAngle; - float lastPitch = 0; - float currentPitch; - bool lastCollisionDebug = 0; - bool currentCollisionDebug = 0; + + // Stores the placement configuration of the text that is currently placed on the screen. + PlacementConfig placedConfig; + + // Stores the placement configuration of how the text should be placed. This isn't necessarily + // the one that is being displayed. + PlacementConfig targetConfig; }; } // namespace mbgl diff --git a/src/mbgl/renderer/symbol_bucket.cpp b/src/mbgl/renderer/symbol_bucket.cpp index d85dbaf8587..281888d9c5c 100644 --- a/src/mbgl/renderer/symbol_bucket.cpp +++ b/src/mbgl/renderer/symbol_bucket.cpp @@ -382,8 +382,8 @@ void SymbolBucket::placeFeatures(CollisionTile& collisionTile, bool swapImmediat // Don't sort symbols that won't overlap because it isn't necessary and // because it causes more labels to pop in and out when rotating. if (mayOverlap) { - float sin = std::sin(collisionTile.angle); - float cos = std::cos(collisionTile.angle); + const float sin = std::sin(collisionTile.config.angle); + const float cos = std::cos(collisionTile.config.angle); std::sort(symbolInstances.begin(), symbolInstances.end(), [sin, cos](SymbolInstance &a, SymbolInstance &b) { const float aRotated = sin * a.x + cos * a.y; @@ -426,8 +426,9 @@ void SymbolBucket::placeFeatures(CollisionTile& collisionTile, bool swapImmediat collisionTile.insertFeature(symbolInstance.textCollisionFeature, glyphScale); } if (glyphScale < collisionTile.maxScale) { - addSymbols(renderDataInProgress->text, - symbolInstance.glyphQuads, glyphScale, layout.text.keep_upright, textAlongLine, collisionTile.angle); + addSymbols( + renderDataInProgress->text, symbolInstance.glyphQuads, glyphScale, + layout.text.keep_upright, textAlongLine, collisionTile.config.angle); } } @@ -436,13 +437,16 @@ void SymbolBucket::placeFeatures(CollisionTile& collisionTile, bool swapImmediat collisionTile.insertFeature(symbolInstance.iconCollisionFeature, iconScale); } if (iconScale < collisionTile.maxScale) { - addSymbols(renderDataInProgress->icon, - symbolInstance.iconQuads, iconScale, layout.icon.keep_upright, iconAlongLine, collisionTile.angle); + addSymbols( + renderDataInProgress->icon, symbolInstance.iconQuads, iconScale, + layout.icon.keep_upright, iconAlongLine, collisionTile.config.angle); } } } - if (collisionTile.getDebug()) addToDebugBuffers(collisionTile); + if (collisionTile.config.debug) { + addToDebugBuffers(collisionTile); + } if (swapImmediately) swapRenderData(); } @@ -513,7 +517,7 @@ void SymbolBucket::addSymbols(Buffer &buffer, const SymbolQuads &symbols, float void SymbolBucket::addToDebugBuffers(CollisionTile &collisionTile) { const float yStretch = collisionTile.yStretch; - const float angle = collisionTile.angle; + const float angle = collisionTile.config.angle; float angle_sin = std::sin(-angle); float angle_cos = std::cos(-angle); std::array matrix = {{angle_cos, -angle_sin, angle_sin, angle_cos}}; @@ -562,7 +566,9 @@ void SymbolBucket::addToDebugBuffers(CollisionTile &collisionTile) { } void SymbolBucket::swapRenderData() { - renderData = std::move(renderDataInProgress); + if (renderDataInProgress) { + renderData = std::move(renderDataInProgress); + } } void SymbolBucket::drawGlyphs(SDFShader &shader) { diff --git a/src/mbgl/text/collision_tile.cpp b/src/mbgl/text/collision_tile.cpp index d18860ccf5a..ecd615a5209 100644 --- a/src/mbgl/text/collision_tile.cpp +++ b/src/mbgl/text/collision_tile.cpp @@ -3,17 +3,16 @@ namespace mbgl { -CollisionTile::CollisionTile(const float angle_, const float pitch, bool debug_) : - angle(angle_), debug(debug_) { +CollisionTile::CollisionTile(PlacementConfig config_) : config(config_) { tree.clear(); - // Compute the transformation matrix. - float angle_sin = std::sin(angle); - float angle_cos = std::cos(angle); - rotationMatrix = {{angle_cos, -angle_sin, angle_sin, angle_cos}}; + // Compute the transformation matrix. + const float angle_sin = std::sin(config.angle); + const float angle_cos = std::cos(config.angle); + rotationMatrix = { { angle_cos, -angle_sin, angle_sin, angle_cos } }; // Stretch boxes in y direction to account for the map tilt. - const float _yStretch = 1.0f / std::cos(pitch); + const float _yStretch = 1.0f / std::cos(config.pitch); // The amount the map is squished depends on the y position. // Sort of account for this by making all boxes a bit bigger. diff --git a/src/mbgl/text/collision_tile.hpp b/src/mbgl/text/collision_tile.hpp index 3fd1b0a4c88..edd5eb61a0e 100644 --- a/src/mbgl/text/collision_tile.hpp +++ b/src/mbgl/text/collision_tile.hpp @@ -2,6 +2,7 @@ #define MBGL_TEXT_COLLISION_TILE #include +#include #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-function" @@ -24,39 +25,34 @@ namespace mbgl { - namespace bg = boost::geometry; - namespace bgm = bg::model; - namespace bgi = bg::index; - typedef bgm::point CollisionPoint; - typedef bgm::box Box; - typedef std::pair CollisionTreeBox; - typedef bgi::rtree> Tree; +namespace bg = boost::geometry; +namespace bgm = bg::model; +namespace bgi = bg::index; +typedef bgm::point CollisionPoint; +typedef bgm::box Box; +typedef std::pair CollisionTreeBox; +typedef bgi::rtree> Tree; class CollisionTile { +public: + explicit CollisionTile(PlacementConfig); - public: - explicit CollisionTile(float angle_, float pitch_, bool debug_); + float placeFeature(const CollisionFeature& feature); + void insertFeature(CollisionFeature& feature, const float minPlacementScale); - float placeFeature(const CollisionFeature &feature); - void insertFeature(CollisionFeature &feature, const float minPlacementScale); - - bool getDebug() { return debug; } - - const float angle = 0; + const PlacementConfig config; const float minScale = 0.5f; const float maxScale = 2.0f; float yStretch; - private: - - Box getTreeBox(const vec2 &anchor, const CollisionBox &box); +private: + Box getTreeBox(const vec2& anchor, const CollisionBox& box); Tree tree; std::array rotationMatrix; - bool debug; - }; -} + +} // namespace mbgl #endif diff --git a/src/mbgl/text/placement_config.hpp b/src/mbgl/text/placement_config.hpp new file mode 100644 index 00000000000..6680f524498 --- /dev/null +++ b/src/mbgl/text/placement_config.hpp @@ -0,0 +1,28 @@ +#ifndef MBGL_TEXT_PLACEMENT_CONFIG +#define MBGL_TEXT_PLACEMENT_CONFIG + +namespace mbgl { + +class PlacementConfig { +public: + inline PlacementConfig(float angle_ = 0, float pitch_ = 0, bool debug_ = false) + : angle(angle_), pitch(pitch_), debug(debug_) { + } + + inline bool operator==(const PlacementConfig& rhs) const { + return angle == rhs.angle && pitch == rhs.pitch && debug == rhs.debug; + } + + inline bool operator!=(const PlacementConfig& rhs) const { + return !operator==(rhs); + } + +public: + float angle; + float pitch; + bool debug; +}; + +} // namespace mbgl + +#endif diff --git a/src/mbgl/util/worker.cpp b/src/mbgl/util/worker.cpp index 371766f0961..4dd6740ec11 100644 --- a/src/mbgl/util/worker.cpp +++ b/src/mbgl/util/worker.cpp @@ -37,10 +37,11 @@ class Worker::Impl { void parseVectorTile(TileWorker* worker, const std::shared_ptr data, + PlacementConfig config, std::function callback) { try { pbf tilePBF(reinterpret_cast(data->data()), data->size()); - callback(worker->parseAllLayers(VectorTile(tilePBF))); + callback(worker->parseAllLayers(VectorTile(tilePBF), config)); } catch (const std::exception& ex) { callback(TileParseResult(ex.what())); } @@ -57,9 +58,10 @@ class Worker::Impl { void parseLiveTile(TileWorker* worker, const AnnotationTile* tile, + PlacementConfig config, std::function callback) { try { - callback(worker->parseAllLayers(*tile)); + callback(worker->parseAllLayers(*tile, config)); } catch (const std::exception& ex) { callback(TileParseResult(ex.what())); } @@ -67,11 +69,9 @@ class Worker::Impl { void redoPlacement(TileWorker* worker, const std::unordered_map>* buckets, - float angle, - float pitch, - bool collisionDebug, + PlacementConfig config, std::function callback) { - worker->redoPlacement(buckets, angle, pitch, collisionDebug); + worker->redoPlacement(buckets, config); callback(); } }; @@ -97,10 +97,11 @@ Worker::parseRasterTile(std::unique_ptr bucket, std::unique_ptr Worker::parseVectorTile(TileWorker& worker, const std::shared_ptr data, + PlacementConfig config, std::function callback) { current = (current + 1) % threads.size(); return threads[current]->invokeWithCallback(&Worker::Impl::parseVectorTile, callback, &worker, - data); + data, config); } std::unique_ptr @@ -113,22 +114,21 @@ Worker::parsePendingVectorTileLayers(TileWorker& worker, std::unique_ptr Worker::parseLiveTile(TileWorker& worker, const AnnotationTile& tile, + PlacementConfig config, std::function callback) { current = (current + 1) % threads.size(); return threads[current]->invokeWithCallback(&Worker::Impl::parseLiveTile, callback, &worker, - &tile); + &tile, config); } std::unique_ptr Worker::redoPlacement(TileWorker& worker, const std::unordered_map>& buckets, - float angle, - float pitch, - bool collisionDebug, + PlacementConfig config, std::function callback) { current = (current + 1) % threads.size(); return threads[current]->invokeWithCallback(&Worker::Impl::redoPlacement, callback, &worker, - &buckets, angle, pitch, collisionDebug); + &buckets, config); } } // end namespace mbgl diff --git a/src/mbgl/util/worker.hpp b/src/mbgl/util/worker.hpp index 7a92a09a514..3d2665e71cf 100644 --- a/src/mbgl/util/worker.hpp +++ b/src/mbgl/util/worker.hpp @@ -37,6 +37,7 @@ class Worker : public mbgl::util::noncopyable { Request parseVectorTile(TileWorker&, std::shared_ptr data, + PlacementConfig config, std::function callback); Request parsePendingVectorTileLayers(TileWorker&, @@ -44,13 +45,12 @@ class Worker : public mbgl::util::noncopyable { Request parseLiveTile(TileWorker&, const AnnotationTile&, + PlacementConfig config, std::function callback); Request redoPlacement(TileWorker&, const std::unordered_map>&, - float angle, - float pitch, - bool collisionDebug, + PlacementConfig config, std::function callback); private: