From 32a2359e86bc86221682f1afa057b62f0f0be8c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Thu, 14 Jan 2016 20:59:17 +0100 Subject: [PATCH] [core] don't store error state Previously, we could first get a stale response from cache, then immediately a "connection error" response that overwrote the error state variable. In the tileLoadingCompleteCallback, we'd only see the second overwritten error state and abort. This could lead to the map sometimes not being rendered at all until the user interacts with the map. --- src/mbgl/map/raster_tile_data.cpp | 6 ++++-- src/mbgl/map/raster_tile_data.hpp | 2 +- src/mbgl/map/source.cpp | 20 +++++++++++++++----- src/mbgl/map/source.hpp | 5 ++++- src/mbgl/map/tile_data.cpp | 4 ---- src/mbgl/map/tile_data.hpp | 11 +---------- src/mbgl/map/vector_tile_data.cpp | 19 ++++++++----------- src/mbgl/map/vector_tile_data.hpp | 4 ++-- 8 files changed, 35 insertions(+), 36 deletions(-) diff --git a/src/mbgl/map/raster_tile_data.cpp b/src/mbgl/map/raster_tile_data.cpp index da2dd808b..92ffb451b 100644 --- a/src/mbgl/map/raster_tile_data.cpp +++ b/src/mbgl/map/raster_tile_data.cpp @@ -27,6 +27,7 @@ void RasterTileData::request(const std::string& url, FileSource* fs = util::ThreadContext::getFileSource(); req = fs->request({ Resource::Kind::Tile, url }, [url, 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(); @@ -36,7 +37,7 @@ void RasterTileData::request(const std::string& url, // This is a different error, e.g. a connection or server error. error = std::make_exception_ptr(std::runtime_error(res.error->message)); } - callback(); + callback(error); return; } @@ -60,6 +61,7 @@ void RasterTileData::request(const std::string& url, return; } + std::exception_ptr error; if (result.is>()) { state = State::parsed; bucket = std::move(result.get>()); @@ -69,7 +71,7 @@ void RasterTileData::request(const std::string& url, bucket.reset(); } - callback(); + callback(error); }); }); } diff --git a/src/mbgl/map/raster_tile_data.hpp b/src/mbgl/map/raster_tile_data.hpp index 34a79cad4..e8efde48e 100644 --- a/src/mbgl/map/raster_tile_data.hpp +++ b/src/mbgl/map/raster_tile_data.hpp @@ -16,7 +16,7 @@ public: RasterTileData(const TileID&, TexturePool&, Worker&); ~RasterTileData(); - using Callback = std::function; + using Callback = std::function; void request(const std::string& url, const Callback& callback); diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index 613972e93..e34ba0748 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -225,7 +225,12 @@ bool Source::handlePartialTile(const TileID& tileID, Worker&) { return true; } - return tileData->parsePending([this, tileID]() { + return tileData->parsePending([this, tileID](std::exception_ptr error) { + if (error) { + observer->onTileError(*this, tileID, error); + return; + } + observer->onTileLoaded(*this, tileID, false); }); } @@ -259,7 +264,9 @@ TileData::State Source::addTile(const TileID& tileID, const StyleUpdateParameter } if (!newTile->data) { - auto callback = std::bind(&Source::tileLoadingCompleteCallback, this, normalizedID, parameters.transformState, parameters.debugOptions & MapDebugOptions::Collision); + auto callback = std::bind(&Source::tileLoadingCompleteCallback, this, normalizedID, + std::placeholders::_1, parameters.transformState, + parameters.debugOptions & MapDebugOptions::Collision); // If we don't find working tile data, we're just going to load it. if (type == SourceType::Raster) { @@ -520,7 +527,10 @@ void Source::setObserver(Observer* observer_) { observer = observer_; } -void Source::tileLoadingCompleteCallback(const TileID& tileID, const TransformState& transformState, bool collisionDebug) { +void Source::tileLoadingCompleteCallback(const TileID& tileID, + std::exception_ptr error, + const TransformState& transformState, + bool collisionDebug) { auto it = tileDataMap.find(tileID); if (it == tileDataMap.end()) { return; @@ -531,8 +541,8 @@ void Source::tileLoadingCompleteCallback(const TileID& tileID, const TransformSt return; } - if (tileData->hasError()) { - observer->onTileError(*this, tileID, tileData->getError()); + if (error) { + observer->onTileError(*this, tileID, error); return; } diff --git a/src/mbgl/map/source.hpp b/src/mbgl/map/source.hpp index f3688ed9e..ade2317cc 100644 --- a/src/mbgl/map/source.hpp +++ b/src/mbgl/map/source.hpp @@ -76,7 +76,10 @@ public: bool enabled = false; private: - void tileLoadingCompleteCallback(const TileID&, const TransformState&, bool collisionDebug); + void tileLoadingCompleteCallback(const TileID&, + std::exception_ptr, + const TransformState&, + bool collisionDebug); bool handlePartialTile(const TileID&, Worker& worker); bool findLoadedChildren(const TileID&, int32_t maxCoveringZoom, std::forward_list& retain); void findLoadedParent(const TileID&, int32_t minCoveringZoom, std::forward_list& retain); diff --git a/src/mbgl/map/tile_data.cpp b/src/mbgl/map/tile_data.cpp index d9fd3f004..edfe4d7ad 100644 --- a/src/mbgl/map/tile_data.cpp +++ b/src/mbgl/map/tile_data.cpp @@ -27,10 +27,6 @@ const char* TileData::StateToString(const State state) { void TileData::dumpDebugLogs() const { Log::Info(Event::General, "TileData::id: %s", std::string(id).c_str()); Log::Info(Event::General, "TileData::state: %s", TileData::StateToString(state)); - - if (error) { - Log::Info(Event::General, "TileData::error: %s", util::toString(error).c_str()); - } } } // namespace mbgl diff --git a/src/mbgl/map/tile_data.hpp b/src/mbgl/map/tile_data.hpp index e31cced96..1067c86d3 100644 --- a/src/mbgl/map/tile_data.hpp +++ b/src/mbgl/map/tile_data.hpp @@ -77,7 +77,7 @@ public: virtual Bucket* getBucket(const StyleLayer&) = 0; - virtual bool parsePending(std::function) { return true; } + virtual bool parsePending(std::function) { return true; } virtual void redoPlacement(PlacementConfig) {} bool isReady() const { @@ -88,14 +88,6 @@ public: return state; } - bool hasError() const { - return error.operator bool(); - } - - std::exception_ptr getError() const { - return error; - } - void dumpDebugLogs() const; const TileID id; @@ -107,7 +99,6 @@ public: protected: std::atomic state; - std::exception_ptr error; }; } // namespace mbgl diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp index 386efe1af..9e1390dec 100644 --- a/src/mbgl/map/vector_tile_data.cpp +++ b/src/mbgl/map/vector_tile_data.cpp @@ -13,7 +13,7 @@ VectorTileData::VectorTileData(const TileID& id_, std::string sourceID, Style& style_, const MapMode mode_, - const std::function& callback) + const std::function& callback) : TileData(id_), style(style_), worker(style_.workers), @@ -32,15 +32,10 @@ VectorTileData::VectorTileData(const TileID& id_, Seconds modified_, Seconds expires_) { if (err) { - error = err; - callback(); + callback(err); return; } - // Reset the error if we didn't receive one for this request. This is to prevent old error - // messages from being displayed again, e.g. after a connection trouble cleared up. - error = nullptr; - modified = modified_; expires = expires_; @@ -49,7 +44,7 @@ VectorTileData::VectorTileData(const TileID& id_, workRequest.reset(); state = State::parsed; buckets.clear(); - callback(); + callback(err); return; } @@ -69,6 +64,7 @@ VectorTileData::VectorTileData(const TileID& id_, return; } + std::exception_ptr error; if (result.is()) { auto& resultBuckets = result.get(); state = resultBuckets.state; @@ -91,7 +87,7 @@ VectorTileData::VectorTileData(const TileID& id_, state = State::obsolete; } - callback(); + callback(error); }); }); } @@ -100,7 +96,7 @@ VectorTileData::~VectorTileData() { cancel(); } -bool VectorTileData::parsePending(std::function callback) { +bool VectorTileData::parsePending(std::function callback) { if (workRequest) { // There's already parsing or placement going on. return false; @@ -113,6 +109,7 @@ bool VectorTileData::parsePending(std::function callback) { return; } + std::exception_ptr error; if (result.is()) { auto& resultBuckets = result.get(); state = resultBuckets.state; @@ -137,7 +134,7 @@ bool VectorTileData::parsePending(std::function callback) { state = State::obsolete; } - callback(); + callback(error); }); return true; diff --git a/src/mbgl/map/vector_tile_data.hpp b/src/mbgl/map/vector_tile_data.hpp index 3d877de60..3ba572f42 100644 --- a/src/mbgl/map/vector_tile_data.hpp +++ b/src/mbgl/map/vector_tile_data.hpp @@ -23,13 +23,13 @@ public: std::string sourceID, Style&, const MapMode, - const std::function& callback); + const std::function& callback); ~VectorTileData(); Bucket* getBucket(const StyleLayer&) override; - bool parsePending(std::function callback) override; + bool parsePending(std::function callback) override; void redoPlacement(PlacementConfig config) override; void redoPlacement();