From e9302c797f68c7e48b908b87b126045c8c5e5209 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 5 Feb 2016 16:52:07 -0800 Subject: [all] Don't interpret 404s on non-tile resources as "no content" --- include/mbgl/storage/response.hpp | 4 + platform/android/src/http_request_android.cpp | 2 + platform/darwin/http_request_nsurl.mm | 2 + platform/default/http_request_curl.cpp | 2 + platform/default/mbgl/storage/offline_database.cpp | 12 +-- src/mbgl/map/map_context.cpp | 9 +- src/mbgl/map/raster_tile_data.cpp | 74 +++++++------- src/mbgl/map/source.cpp | 108 ++++++++++----------- src/mbgl/map/vector_tile.cpp | 21 ++-- src/mbgl/sprite/sprite_store.cpp | 26 ++--- src/mbgl/storage/response.cpp | 1 + src/mbgl/text/glyph_pbf.cpp | 26 ++--- test/storage/http_reading.cpp | 50 ++++++++++ test/storage/offline_database.cpp | 14 +-- test/storage/server.js | 4 + test/style/source.cpp | 58 +++++++++++ 16 files changed, 256 insertions(+), 157 deletions(-) diff --git a/include/mbgl/storage/response.hpp b/include/mbgl/storage/response.hpp index 278fce1dfe..162a272948 100644 --- a/include/mbgl/storage/response.hpp +++ b/include/mbgl/storage/response.hpp @@ -20,6 +20,10 @@ public: // When this object is empty, the response was successful. std::unique_ptr error; + // This is set to true for 204 Not Modified responses, and, for backward compatibility, + // for 404 Not Found responses for tiles. + bool noContent = false; + // This is set to true for 304 Not Modified responses. bool notModified = false; diff --git a/platform/android/src/http_request_android.cpp b/platform/android/src/http_request_android.cpp index 105ba5860c..023e0cde97 100644 --- a/platform/android/src/http_request_android.cpp +++ b/platform/android/src/http_request_android.cpp @@ -205,6 +205,8 @@ void HTTPAndroidRequest::onResponse(JNIEnv* env, int code, jstring /* message */ response->data = std::make_shared(reinterpret_cast(bodyData), env->GetArrayLength(body)); env->ReleaseByteArrayElements(body, bodyData, JNI_ABORT); } + } else if (code == 204 || (code == 404 && resource.kind == Resource::Kind::Tile)) { + response->noContent = true; } else if (code == 304) { response->notModified = true; } else if (code == 404) { diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm index 890e347afc..bc368f8724 100644 --- a/platform/darwin/http_request_nsurl.mm +++ b/platform/darwin/http_request_nsurl.mm @@ -217,6 +217,8 @@ void HTTPNSURLRequest::handleResult(NSData *data, NSURLResponse *res, NSError *e if (responseCode == 200) { response->data = std::make_shared((const char *)[data bytes], [data length]); + } else if (responseCode == 204 || (responseCode == 404 && resource.kind == Resource::Kind::Tile)) { + response->noContent = true; } else if (responseCode == 304) { response->notModified = true; } else if (responseCode == 404) { diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp index 660da4c43e..5b092573c6 100644 --- a/platform/default/http_request_curl.cpp +++ b/platform/default/http_request_curl.cpp @@ -515,6 +515,8 @@ void HTTPCURLRequest::handleResult(CURLcode code) { } else { response->data = std::make_shared(); } + } else if (responseCode == 204 || (responseCode == 404 && resource.kind == Resource::Kind::Tile)) { + response->noContent = true; } else if (responseCode == 304) { response->notModified = true; } else if (responseCode == 404) { diff --git a/platform/default/mbgl/storage/offline_database.cpp b/platform/default/mbgl/storage/offline_database.cpp index cd0f88b7fe..67fcc435c6 100644 --- a/platform/default/mbgl/storage/offline_database.cpp +++ b/platform/default/mbgl/storage/offline_database.cpp @@ -101,8 +101,8 @@ optional OfflineDatabase::get(const Resource& resource) { } void OfflineDatabase::put(const Resource& resource, const Response& response) { - // Except for 404s, don't store errors in the cache. - if (response.error && response.error->reason != Response::Error::Reason::NotFound) { + // Don't store errors in the cache. + if (response.error) { return; } @@ -135,7 +135,7 @@ optional OfflineDatabase::getResource(const Resource& resource) { optional data = stmt.get>(3); if (!data) { - response.error = std::make_unique(Response::Error::Reason::NotFound); + response.noContent = true; } else if (stmt.get(4)) { response.data = std::make_shared(util::decompress(*data)); } else { @@ -170,7 +170,7 @@ void OfflineDatabase::putResource(const Resource& resource, const Response& resp std::string data; - if (response.error) { // Can only be NotFound + if (response.noContent) { stmt.bind(7 /* data */, nullptr); stmt.bind(8 /* compressed */, false); } else { @@ -218,7 +218,7 @@ optional OfflineDatabase::getTile(const Resource::TileData& tile) { optional data = stmt.get>(3); if (!data) { - response.error = std::make_unique(Response::Error::Reason::NotFound); + response.noContent = true; } else if (stmt.get(4)) { response.data = std::make_shared(util::decompress(*data)); } else { @@ -276,7 +276,7 @@ void OfflineDatabase::putTile(const Resource::TileData& tile, const Response& re std::string data; - if (response.error) { // Can only be NotFound + if (response.noContent) { stmt2.bind(10 /* data */, nullptr); stmt2.bind(11 /* compressed */, false); } else { diff --git a/src/mbgl/map/map_context.cpp b/src/mbgl/map/map_context.cpp index 03da5ee321..6b4657f0af 100644 --- a/src/mbgl/map/map_context.cpp +++ b/src/mbgl/map/map_context.cpp @@ -113,14 +113,11 @@ void MapContext::setStyleURL(const std::string& url) { Log::Error(Event::Setup, "loading style failed: %s", res.error->message.c_str()); data.loading = false; } + } else if (res.notModified || res.noContent) { return; + } else { + loadStyleJSON(*res.data, base); } - - if (res.notModified) { - return; - } - - loadStyleJSON(*res.data, base); }); } diff --git a/src/mbgl/map/raster_tile_data.cpp b/src/mbgl/map/raster_tile_data.cpp index f9f5480197..8df4e05b62 100644 --- a/src/mbgl/map/raster_tile_data.cpp +++ b/src/mbgl/map/raster_tile_data.cpp @@ -22,52 +22,46 @@ RasterTileData::RasterTileData(const TileID& id_, const Resource resource = Resource::tile(urlTemplate, pixelRatio, id.x, id.y, id.sourceZ); req = util::ThreadContext::getFileSource()->request(resource, [callback, this](Response res) { if (res.error) { - std::exception_ptr error; - if (res.error->reason == Response::Error::Reason::NotFound) { - // This is a 404 response. We're treating these as empty tiles. - workRequest.reset(); - state = State::parsed; - bucket.reset(); - } else { - // This is a different error, e.g. a connection or server error. - error = std::make_exception_ptr(std::runtime_error(res.error->message)); - } - callback(error); - return; - } - - modified = res.modified; - expires = res.expires; - - if (res.notModified) { - // We got the same data again. Abort early. - return; - } + callback(std::make_exception_ptr(std::runtime_error(res.error->message))); + } else if (res.notModified) { + modified = res.modified; + expires = res.expires; + } else if (res.noContent) { + state = State::parsed; + modified = res.modified; + expires = res.expires; + workRequest.reset(); + bucket.reset(); + callback(nullptr); + } else { + modified = res.modified; + expires = res.expires; - if (state == State::loading) { // Only overwrite the state when we didn't have a previous tile. - state = State::loaded; - } + if (state == State::loading) { + state = State::loaded; + } - workRequest.reset(); - workRequest = worker.parseRasterTile(std::make_unique(texturePool), res.data, [this, callback] (RasterTileParseResult result) { workRequest.reset(); - if (state != State::loaded) { - return; - } + workRequest = worker.parseRasterTile(std::make_unique(texturePool), res.data, [this, callback] (RasterTileParseResult result) { + workRequest.reset(); + if (state != State::loaded) { + return; + } - std::exception_ptr error; - if (result.is>()) { - state = State::parsed; - bucket = std::move(result.get>()); - } else { - error = result.get(); - state = State::obsolete; - bucket.reset(); - } + std::exception_ptr error; + if (result.is>()) { + state = State::parsed; + bucket = std::move(result.get>()); + } else { + error = result.get(); + state = State::obsolete; + bucket.reset(); + } - callback(error); - }); + callback(error); + }); + } }); } diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index e763674040..1a7933f53e 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -90,76 +90,74 @@ void Source::load() { req = fs->request(Resource::source(url), [this](Response res) { if (res.error) { observer->onSourceError(*this, std::make_exception_ptr(std::runtime_error(res.error->message))); + } else if (res.notModified) { return; - } + } else if (res.noContent) { + observer->onSourceError(*this, std::make_exception_ptr(std::runtime_error("unexpectedly empty source"))); + } else { + bool reloadTiles = false; + + if (type == SourceType::Vector || type == SourceType::Raster) { + std::unique_ptr newInfo; + + // Create a new copy of the SourceInfo object that holds the base values we've parsed + // from the stylesheet. Then merge in the values parsed from the TileJSON we retrieved + // via the URL. + try { + newInfo = StyleParser::parseTileJSON(*res.data, url, type); + } catch (...) { + observer->onSourceError(*this, std::current_exception()); + return; + } - if (res.notModified) { - // We got the same data back as last time. Abort early. - return; - } + // Check whether previous information specifies different tile + if (info && info->tiles != newInfo->tiles) { + reloadTiles = true; - bool reloadTiles = false; + // Tile size changed: We need to recalculate the tiles we need to load because we + // might have to load tiles for a different zoom level + // This is done automatically when we trigger the onSourceLoaded observer below. - if (type == SourceType::Vector || type == SourceType::Raster) { - std::unique_ptr newInfo; + // Min/Max zoom changed: We need to recalculate what tiles to load, if we have tiles + // loaded that are outside the new zoom range + // This is done automatically when we trigger the onSourceLoaded observer below. - // Create a new copy of the SourceInfo object that holds the base values we've parsed - // from the stylesheet. Then merge in the values parsed from the TileJSON we retrieved - // via the URL. - try { - newInfo = StyleParser::parseTileJSON(*res.data, url, type); - } catch (...) { - observer->onSourceError(*this, std::current_exception()); - return; - } + // Attribution changed: We need to notify the embedding application that this + // changed. See https://github.com/mapbox/mapbox-gl-native/issues/2723 + // This is not yet implemented. - // Check whether previous information specifies different tile - if (info && info->tiles != newInfo->tiles) { - reloadTiles = true; + // Center/bounds changed: We're not using these values currently + } - // Tile size changed: We need to recalculate the tiles we need to load because we - // might have to load tiles for a different zoom level - // This is done automatically when we trigger the onSourceLoaded observer below. + info = std::move(newInfo); + } else if (type == SourceType::GeoJSON) { + info = std::make_unique(); - // Min/Max zoom changed: We need to recalculate what tiles to load, if we have tiles - // loaded that are outside the new zoom range - // This is done automatically when we trigger the onSourceLoaded observer below. + rapidjson::GenericDocument, rapidjson::CrtAllocator> d; + d.Parse<0>(res.data->c_str()); - // Attribution changed: We need to notify the embedding application that this - // changed. See https://github.com/mapbox/mapbox-gl-native/issues/2723 - // This is not yet implemented. + if (d.HasParseError()) { + std::stringstream message; + message << d.GetErrorOffset() << " - " << rapidjson::GetParseError_En(d.GetParseError()); + observer->onSourceError(*this, std::make_exception_ptr(std::runtime_error(message.str()))); + return; + } - // Center/bounds changed: We're not using these values currently + geojsonvt = StyleParser::parseGeoJSON(d); + reloadTiles = true; } - info = std::move(newInfo); - } else if (type == SourceType::GeoJSON) { - info = std::make_unique(); - - rapidjson::GenericDocument, rapidjson::CrtAllocator> d; - d.Parse<0>(res.data->c_str()); - - if (d.HasParseError()) { - std::stringstream message; - message << d.GetErrorOffset() << " - " << rapidjson::GetParseError_En(d.GetParseError()); - observer->onSourceError(*this, std::make_exception_ptr(std::runtime_error(message.str()))); - return; + if (reloadTiles) { + // Tile information changed because we got new GeoJSON data, or a new tile URL. + tilePtrs.clear(); + tileDataMap.clear(); + tiles.clear(); + cache.clear(); } - geojsonvt = StyleParser::parseGeoJSON(d); - reloadTiles = true; + loaded = true; + observer->onSourceLoaded(*this); } - - if (reloadTiles) { - // Tile information changed because we got new GeoJSON data, or a new tile URL. - tilePtrs.clear(); - tileDataMap.clear(); - tiles.clear(); - cache.clear(); - } - - loaded = true; - observer->onSourceLoaded(*this); }); } diff --git a/src/mbgl/map/vector_tile.cpp b/src/mbgl/map/vector_tile.cpp index 17746a26a1..6de3b76b4f 100644 --- a/src/mbgl/map/vector_tile.cpp +++ b/src/mbgl/map/vector_tile.cpp @@ -191,22 +191,15 @@ VectorTileMonitor::VectorTileMonitor(const TileID& tileID_, float pixelRatio_, c std::unique_ptr VectorTileMonitor::monitorTile(const GeometryTileMonitor::Callback& callback) { const Resource resource = Resource::tile(urlTemplate, pixelRatio, tileID.x, tileID.y, tileID.sourceZ); return util::ThreadContext::getFileSource()->request(resource, [callback, this](Response res) { - if (res.notModified) { - // We got the same data again. Abort early. - return; - } - if (res.error) { - if (res.error->reason == Response::Error::Reason::NotFound) { - callback(nullptr, nullptr, res.modified, res.expires); - return; - } else { - callback(std::make_exception_ptr(std::runtime_error(res.error->message)), nullptr, res.modified, res.expires); - return; - } + callback(std::make_exception_ptr(std::runtime_error(res.error->message)), nullptr, res.modified, res.expires); + } else if (res.notModified) { + return; + } else if (res.noContent) { + callback(nullptr, nullptr, res.modified, res.expires); + } else { + callback(nullptr, std::make_unique(res.data), res.modified, res.expires); } - - callback(nullptr, std::make_unique(res.data), res.modified, res.expires); }); } diff --git a/src/mbgl/sprite/sprite_store.cpp b/src/mbgl/sprite/sprite_store.cpp index 15a345cc4d..38aa7009ca 100644 --- a/src/mbgl/sprite/sprite_store.cpp +++ b/src/mbgl/sprite/sprite_store.cpp @@ -38,15 +38,12 @@ void SpriteStore::setURL(const std::string& url) { loader->jsonRequest = fs->request(Resource::spriteJSON(url, pixelRatio), [this](Response res) { if (res.error) { observer->onSpriteError(std::make_exception_ptr(std::runtime_error(res.error->message))); + } else if (res.notModified) { return; - } - - if (res.notModified) { - // We got the same data back as last time. Abort early. - return; - } - - if (!loader->json || *loader->json != *res.data) { + } else if (res.noContent) { + loader->json = std::make_shared(); + emitSpriteLoadedIfComplete(); + } else { // Only trigger a sprite loaded event we got new data. loader->json = res.data; emitSpriteLoadedIfComplete(); @@ -56,15 +53,12 @@ void SpriteStore::setURL(const std::string& url) { loader->spriteRequest = fs->request(Resource::spriteImage(url, pixelRatio), [this](Response res) { if (res.error) { observer->onSpriteError(std::make_exception_ptr(std::runtime_error(res.error->message))); + } else if (res.notModified) { return; - } - - if (res.notModified) { - // We got the same data back as last time. Abort early. - return; - } - - if (!loader->image || *loader->image != *res.data) { + } else if (res.noContent) { + loader->image = std::make_shared(); + emitSpriteLoadedIfComplete(); + } else { loader->image = res.data; emitSpriteLoadedIfComplete(); } diff --git a/src/mbgl/storage/response.cpp b/src/mbgl/storage/response.cpp index 22263d2ebb..09c43c8a6a 100644 --- a/src/mbgl/storage/response.cpp +++ b/src/mbgl/storage/response.cpp @@ -9,6 +9,7 @@ Response::Response(const Response& res) { Response& Response::operator=(const Response& res) { error = res.error ? std::make_unique(*res.error) : nullptr; + noContent = res.noContent; notModified = res.notModified; data = res.data; modified = res.modified; diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp index 0dfed6b430..5973c0ce14 100644 --- a/src/mbgl/text/glyph_pbf.cpp +++ b/src/mbgl/text/glyph_pbf.cpp @@ -71,22 +71,22 @@ GlyphPBF::GlyphPBF(GlyphStore* store, req = fs->request(Resource::glyphs(store->getURL(), fontStack, glyphRange), [this, store, fontStack, glyphRange](Response res) { if (res.error) { observer->onGlyphsError(fontStack, glyphRange, std::make_exception_ptr(std::runtime_error(res.error->message))); + } else if (res.notModified) { return; - } - - if (res.notModified) { - return; - } + } else if (res.noContent) { + parsed = true; + observer->onGlyphsLoaded(fontStack, glyphRange); + } else { + try { + parseGlyphPBF(**store->getFontStack(fontStack), *res.data); + } catch (...) { + observer->onGlyphsError(fontStack, glyphRange, std::current_exception()); + return; + } - try { - parseGlyphPBF(**store->getFontStack(fontStack), *res.data); - } catch (...) { - observer->onGlyphsError(fontStack, glyphRange, std::current_exception()); - return; + parsed = true; + observer->onGlyphsLoaded(fontStack, glyphRange); } - - parsed = true; - observer->onGlyphsLoaded(fontStack, glyphRange); }); } diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp index d360ec0031..b8e474ee43 100644 --- a/test/storage/http_reading.cpp +++ b/test/storage/http_reading.cpp @@ -59,6 +59,56 @@ TEST_F(Storage, HTTP404) { loop.run(); } +TEST_F(Storage, HTTPTile404) { + SCOPED_TEST(HTTPTile404) + + using namespace mbgl; + + util::RunLoop loop; + OnlineFileSource fs; + + std::unique_ptr req2 = fs.request({ Resource::Tile, "http://127.0.0.1:3000/doesnotexist" }, + [&](Response res) { + req2.reset(); + EXPECT_TRUE(util::ThreadContext::currentlyOn(util::ThreadType::Main)); + EXPECT_TRUE(res.noContent); + EXPECT_FALSE(bool(res.error)); + EXPECT_FALSE(bool(res.data)); + EXPECT_FALSE(bool(res.expires)); + EXPECT_FALSE(bool(res.modified)); + EXPECT_FALSE(bool(res.etag)); + loop.stop(); + HTTPTile404.finish(); + }); + + loop.run(); +} + +TEST_F(Storage, HTTP204) { + SCOPED_TEST(HTTP204) + + using namespace mbgl; + + util::RunLoop loop; + OnlineFileSource fs; + + std::unique_ptr req2 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/no-content" }, + [&](Response res) { + req2.reset(); + EXPECT_TRUE(util::ThreadContext::currentlyOn(util::ThreadType::Main)); + EXPECT_TRUE(res.noContent); + EXPECT_FALSE(bool(res.error)); + EXPECT_FALSE(bool(res.data)); + EXPECT_FALSE(bool(res.expires)); + EXPECT_FALSE(bool(res.modified)); + EXPECT_FALSE(bool(res.etag)); + loop.stop(); + HTTP204.finish(); + }); + + loop.run(); +} + TEST_F(Storage, HTTP500) { SCOPED_TEST(HTTP500) diff --git a/test/storage/offline_database.cpp b/test/storage/offline_database.cpp index e2e32ee36b..1c43506066 100644 --- a/test/storage/offline_database.cpp +++ b/test/storage/offline_database.cpp @@ -334,19 +334,19 @@ TEST(OfflineDatabase, PutTile) { EXPECT_EQ("data", *res->data); } -TEST(OfflineDatabase, PutResourceNotFound) { +TEST(OfflineDatabase, PutResourceNoContent) { using namespace mbgl; OfflineDatabase db(":memory:"); Resource resource { Resource::Style, "http://example.com/" }; Response response; - response.error = std::make_unique(Response::Error::Reason::NotFound); + response.noContent = true; db.put(resource, response); auto res = db.get(resource); - EXPECT_NE(nullptr, res->error); - EXPECT_EQ(Response::Error::Reason::NotFound, res->error->reason); + EXPECT_EQ(nullptr, res->error); + EXPECT_TRUE(res->noContent); EXPECT_FALSE(res->data.get()); } @@ -364,11 +364,11 @@ TEST(OfflineDatabase, PutTileNotFound) { 0 }; Response response; - response.error = std::make_unique(Response::Error::Reason::NotFound); + response.noContent = true; db.put(resource, response); auto res = db.get(resource); - EXPECT_NE(nullptr, res->error); - EXPECT_EQ(Response::Error::Reason::NotFound, res->error->reason); + EXPECT_EQ(nullptr, res->error); + EXPECT_TRUE(res->noContent); EXPECT_FALSE(res->data.get()); } diff --git a/test/storage/server.js b/test/storage/server.js index 0024330037..16b507daa9 100755 --- a/test/storage/server.js +++ b/test/storage/server.js @@ -84,6 +84,10 @@ app.get('/revalidate-etag', function(req, res) { revalidateEtagCounter++; }); +app.get('/no-content', function(req, res) { + res.status(204).send(); +}); + app.get('/not-found', function(req, res) { res.status(404).send('Not Found!'); }); diff --git a/test/style/source.cpp b/test/style/source.cpp index 528059ae9f..f5b46292ab 100644 --- a/test/style/source.cpp +++ b/test/style/source.cpp @@ -116,6 +116,64 @@ TEST(Source, LoadingCorrupt) { test.run(); } +TEST(Source, RasterTileEmpty) { + SourceTest test; + + test.fileSource.tileResponse = [&] (const Resource&) { + Response response; + response.noContent = true; + return response; + }; + + test.observer.tileLoaded = [&] (Source& source, const TileID&, bool) { + EXPECT_EQ("source", source.id); + test.end(); + }; + + test.observer.tileError = [&] (Source&, const TileID&, std::exception_ptr) { + FAIL() << "Should never be called"; + }; + + auto info = std::make_unique(); + info->tiles = { "tiles" }; + + Source source(SourceType::Raster, "source", "", 512, std::move(info), nullptr); + source.setObserver(&test.observer); + source.load(); + source.update(test.updateParameters); + + test.run(); +} + +TEST(Source, VectorTileEmpty) { + SourceTest test; + + test.fileSource.tileResponse = [&] (const Resource&) { + Response response; + response.noContent = true; + return response; + }; + + test.observer.tileLoaded = [&] (Source& source, const TileID&, bool) { + EXPECT_EQ("source", source.id); + test.end(); + }; + + test.observer.tileError = [&] (Source&, const TileID&, std::exception_ptr) { + FAIL() << "Should never be called"; + }; + + auto info = std::make_unique(); + info->tiles = { "tiles" }; + + Source source(SourceType::Vector, "source", "", 512, std::move(info), nullptr); + source.setObserver(&test.observer); + source.load(); + source.update(test.updateParameters); + + test.run(); +} + TEST(Source, RasterTileFail) { SourceTest test; -- cgit v1.2.1