Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Commit

Permalink
[all] Don't interpret 404s on non-tile resources as "no content"
Browse files Browse the repository at this point in the history
  • Loading branch information
@jfirebaugh
jfirebaugh committed Feb 10, 2016
1 parent 7eb1a91 commit e9302c7
Show file tree
Hide file tree
Showing 16 changed files with 256 additions and 157 deletions.
4 changes: 4 additions & 0 deletions include/mbgl/storage/response.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ class Response {
// When this object is empty, the response was successful.
std::unique_ptr<const Error> 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;

Expand Down
2 changes: 2 additions & 0 deletions platform/android/src/http_request_android.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,8 @@ void HTTPAndroidRequest::onResponse(JNIEnv* env, int code, jstring /* message */
response->data = std::make_shared<std::string>(reinterpret_cast<char*>(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) {
Expand Down
2 changes: 2 additions & 0 deletions platform/darwin/http_request_nsurl.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,8 @@

if (responseCode == 200) {
response->data = std::make_shared<std::string>((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) {
Expand Down
2 changes: 2 additions & 0 deletions platform/default/http_request_curl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,8 @@ void HTTPCURLRequest::handleResult(CURLcode code) {
} else {
response->data = std::make_shared<std::string>();
}
} 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) {
Expand Down
12 changes: 6 additions & 6 deletions platform/default/mbgl/storage/offline_database.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ optional<Response> 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) {

This comment has been minimized.

Sign in to view

Copy link
@kkaefer

kkaefer Feb 22, 2017

Contributor

This means we're no longer storing 404 responses in the cache. What is the motivation behind doing that?

This comment has been minimized.

Sign in to view

Copy link
@jfirebaugh

jfirebaugh Feb 22, 2017

Author Contributor

I think I assumed we were previously storing 404 responses only due to the use of 404 to indicate "no content" for tiles. If it's important to cache 404s, we can probably add them back.
return;
}

Expand Down Expand Up @@ -135,7 +135,7 @@ optional<Response> OfflineDatabase::getResource(const Resource& resource) {

optional<std::string> data = stmt.get<optional<std::string>>(3);
if (!data) {
response.error = std::make_unique<Response::Error>(Response::Error::Reason::NotFound);
response.noContent = true;
} else if (stmt.get<int>(4)) {
response.data = std::make_shared<std::string>(util::decompress(*data));
} else {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -218,7 +218,7 @@ optional<Response> OfflineDatabase::getTile(const Resource::TileData& tile) {

optional<std::string> data = stmt.get<optional<std::string>>(3);
if (!data) {
response.error = std::make_unique<Response::Error>(Response::Error::Reason::NotFound);
response.noContent = true;
} else if (stmt.get<int>(4)) {
response.data = std::make_shared<std::string>(util::decompress(*data));
} else {
Expand Down Expand Up @@ -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 {
Expand Down
9 changes: 3 additions & 6 deletions src/mbgl/map/map_context.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}

Expand Down
74 changes: 34 additions & 40 deletions src/mbgl/map/raster_tile_data.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<RasterBucket>(texturePool), res.data, [this, callback] (RasterTileParseResult result) {
workRequest.reset();
if (state != State::loaded) {
return;
}
workRequest = worker.parseRasterTile(std::make_unique<RasterBucket>(texturePool), res.data, [this, callback] (RasterTileParseResult result) {
workRequest.reset();
if (state != State::loaded) {
return;
}

std::exception_ptr error;
if (result.is<std::unique_ptr<Bucket>>()) {
state = State::parsed;
bucket = std::move(result.get<std::unique_ptr<Bucket>>());
} else {
error = result.get<std::exception_ptr>();
state = State::obsolete;
bucket.reset();
}
std::exception_ptr error;
if (result.is<std::unique_ptr<Bucket>>()) {
state = State::parsed;
bucket = std::move(result.get<std::unique_ptr<Bucket>>());
} else {
error = result.get<std::exception_ptr>();
state = State::obsolete;
bucket.reset();
}

callback(error);
});
callback(error);
});
}
});
}

Expand Down
108 changes: 53 additions & 55 deletions src/mbgl/map/source.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<SourceInfo> 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<SourceInfo> 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<SourceInfo>();

// 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::UTF8<>, 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<SourceInfo>();

rapidjson::GenericDocument<rapidjson::UTF8<>, 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);
});
}

Expand Down
21 changes: 7 additions & 14 deletions src/mbgl/map/vector_tile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,22 +191,15 @@ VectorTileMonitor::VectorTileMonitor(const TileID& tileID_, float pixelRatio_, c
std::unique_ptr<FileRequest> 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<VectorTile>(res.data), res.modified, res.expires);
}

callback(nullptr, std::make_unique<VectorTile>(res.data), res.modified, res.expires);
});
}

Expand Down
26 changes: 10 additions & 16 deletions src/mbgl/sprite/sprite_store.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<const std::string>();
emitSpriteLoadedIfComplete();
} else {
// Only trigger a sprite loaded event we got new data.
loader->json = res.data;
emitSpriteLoadedIfComplete();
Expand All @@ -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<const std::string>();
emitSpriteLoadedIfComplete();
} else {
loader->image = res.data;
emitSpriteLoadedIfComplete();
}
Expand Down
1 change: 1 addition & 0 deletions src/mbgl/storage/response.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Response::Response(const Response& res) {

Response& Response::operator=(const Response& res) {
error = res.error ? std::make_unique<Error>(*res.error) : nullptr;
noContent = res.noContent;
notModified = res.notModified;
data = res.data;
modified = res.modified;
Expand Down
Loading

0 comments on commit e9302c7

Please sign in to comment.