From 77a39a1ed83aa4bce916df791b6bb91cc2169101 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Sat, 16 Sep 2017 00:44:33 +0200 Subject: [core] make sure tiles are not treated as complete until all worker operations completed Previously, when we started a worker operation that eventually throws an exception (e.g. due to the tile not being parseable), and then enqueue another worker operation while the first one is processing, we treated the worker as idle once the first operation's error callback fired, even though the second operation was still in progress. Due to our use of coalescing, I was unable to come up with a reliable test since we'd need to reproduce the behavior described above, which is timing dependent. --- src/mbgl/tile/geometry_tile.cpp | 13 ++++++++----- src/mbgl/tile/geometry_tile.hpp | 8 +++----- src/mbgl/tile/geometry_tile_worker.cpp | 18 ++++++++---------- src/mbgl/tile/raster_tile.cpp | 15 ++++++++++++--- src/mbgl/tile/raster_tile.hpp | 6 ++++-- src/mbgl/tile/raster_tile_worker.cpp | 8 ++++---- src/mbgl/tile/raster_tile_worker.hpp | 2 +- test/tile/annotation_tile.test.cpp | 9 +++------ test/tile/geojson_tile.test.cpp | 14 +++++++------- test/tile/raster_tile.test.cpp | 6 +++--- test/tile/vector_tile.test.cpp | 9 ++++----- 11 files changed, 57 insertions(+), 51 deletions(-) diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index c4d5d3bae3..23b3737a38 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -116,9 +116,10 @@ void GeometryTile::redoLayout() { worker.invoke(&GeometryTileWorker::setLayers, std::move(copy), correlationID); } -void GeometryTile::onLayout(LayoutResult result) { +void GeometryTile::onLayout(LayoutResult result, const uint64_t resultCorrelationID) { loaded = true; renderable = true; + (void)resultCorrelationID; nonSymbolBuckets = std::move(result.nonSymbolBuckets); featureIndex = std::move(result.featureIndex); data = std::move(result.tileData); @@ -126,10 +127,10 @@ void GeometryTile::onLayout(LayoutResult result) { observer->onTileChanged(*this); } -void GeometryTile::onPlacement(PlacementResult result) { +void GeometryTile::onPlacement(PlacementResult result, const uint64_t resultCorrelationID) { loaded = true; renderable = true; - if (result.correlationID == correlationID) { + if (resultCorrelationID == correlationID) { pending = false; } symbolBuckets = std::move(result.symbolBuckets); @@ -140,9 +141,11 @@ void GeometryTile::onPlacement(PlacementResult result) { observer->onTileChanged(*this); } -void GeometryTile::onError(std::exception_ptr err) { +void GeometryTile::onError(std::exception_ptr err, const uint64_t resultCorrelationID) { loaded = true; - pending = false; + if (resultCorrelationID == correlationID) { + pending = false; + } observer->onTileError(*this, err); } diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index ed5d8d87bf..910cb446a0 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -68,19 +68,17 @@ public: std::unordered_map> nonSymbolBuckets; std::unique_ptr featureIndex; std::unique_ptr tileData; - uint64_t correlationID; }; - void onLayout(LayoutResult); + void onLayout(LayoutResult, uint64_t correlationID); class PlacementResult { public: std::unordered_map> symbolBuckets; std::unique_ptr collisionTile; - uint64_t correlationID; }; - void onPlacement(PlacementResult); + void onPlacement(PlacementResult, uint64_t correlationID); - void onError(std::exception_ptr); + void onError(std::exception_ptr, uint64_t correlationID); protected: const GeometryTileData* getData() { diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index 616a0bba1f..25834914c4 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -88,7 +88,7 @@ void GeometryTileWorker::setData(std::unique_ptr data_, break; } } catch (...) { - parent.invoke(&GeometryTile::onError, std::current_exception()); + parent.invoke(&GeometryTile::onError, std::current_exception(), correlationID); } } @@ -112,7 +112,7 @@ void GeometryTileWorker::setLayers(std::vector> layers_, break; } } catch (...) { - parent.invoke(&GeometryTile::onError, std::current_exception()); + parent.invoke(&GeometryTile::onError, std::current_exception(), correlationID); } } @@ -136,7 +136,7 @@ void GeometryTileWorker::setPlacementConfig(PlacementConfig placementConfig_, ui break; } } catch (...) { - parent.invoke(&GeometryTile::onError, std::current_exception()); + parent.invoke(&GeometryTile::onError, std::current_exception(), correlationID); } } @@ -161,7 +161,7 @@ void GeometryTileWorker::symbolDependenciesChanged() { break; } } catch (...) { - parent.invoke(&GeometryTile::onError, std::current_exception()); + parent.invoke(&GeometryTile::onError, std::current_exception(), correlationID); } } @@ -187,7 +187,7 @@ void GeometryTileWorker::coalesced() { break; } } catch (...) { - parent.invoke(&GeometryTile::onError, std::current_exception()); + parent.invoke(&GeometryTile::onError, std::current_exception(), correlationID); } } @@ -353,8 +353,7 @@ void GeometryTileWorker::redoLayout() { std::move(buckets), std::move(featureIndex), *data ? (*data)->clone() : nullptr, - correlationID - }); + }, correlationID); attemptPlacement(); } @@ -409,9 +408,8 @@ void GeometryTileWorker::attemptPlacement() { parent.invoke(&GeometryTile::onPlacement, GeometryTile::PlacementResult { std::move(buckets), - std::move(collisionTile), - correlationID - }); + std::move(collisionTile), + }, correlationID); } } // namespace mbgl diff --git a/src/mbgl/tile/raster_tile.cpp b/src/mbgl/tile/raster_tile.cpp index 770561e703..12dfaf87d4 100644 --- a/src/mbgl/tile/raster_tile.cpp +++ b/src/mbgl/tile/raster_tile.cpp @@ -37,18 +37,27 @@ void RasterTile::setData(std::shared_ptr data, optional expires_) { modified = modified_; expires = expires_; - worker.invoke(&RasterTileWorker::parse, data); + + pending = true; + ++correlationID; + worker.invoke(&RasterTileWorker::parse, data, correlationID); } -void RasterTile::onParsed(std::unique_ptr result) { +void RasterTile::onParsed(std::unique_ptr result, const uint64_t resultCorrelationID) { bucket = std::move(result); loaded = true; + if (resultCorrelationID == correlationID) { + pending = false; + } renderable = bucket ? true : false; observer->onTileChanged(*this); } -void RasterTile::onError(std::exception_ptr err) { +void RasterTile::onError(std::exception_ptr err, const uint64_t resultCorrelationID) { loaded = true; + if (resultCorrelationID == correlationID) { + pending = false; + } observer->onTileError(*this, err); } diff --git a/src/mbgl/tile/raster_tile.hpp b/src/mbgl/tile/raster_tile.hpp index e047430485..28fcb554a9 100644 --- a/src/mbgl/tile/raster_tile.hpp +++ b/src/mbgl/tile/raster_tile.hpp @@ -31,8 +31,8 @@ public: void cancel() override; Bucket* getBucket(const RenderLayer&) const override; - void onParsed(std::unique_ptr result); - void onError(std::exception_ptr); + void onParsed(std::unique_ptr result, uint64_t correlationID); + void onError(std::exception_ptr, uint64_t correlationID); private: TileLoader loader; @@ -40,6 +40,8 @@ private: std::shared_ptr mailbox; Actor worker; + uint64_t correlationID = 0; + // Contains the Bucket object for the tile. Buckets are render // objects and they get added by tile parsing operations. std::unique_ptr bucket; diff --git a/src/mbgl/tile/raster_tile_worker.cpp b/src/mbgl/tile/raster_tile_worker.cpp index 8c1fc2f673..585fe5ec95 100644 --- a/src/mbgl/tile/raster_tile_worker.cpp +++ b/src/mbgl/tile/raster_tile_worker.cpp @@ -10,17 +10,17 @@ RasterTileWorker::RasterTileWorker(ActorRef, ActorRef data) { +void RasterTileWorker::parse(std::shared_ptr data, uint64_t correlationID) { if (!data) { - parent.invoke(&RasterTile::onParsed, nullptr); // No data; empty tile. + parent.invoke(&RasterTile::onParsed, nullptr, correlationID); // No data; empty tile. return; } try { auto bucket = std::make_unique(util::unpremultiply(decodeImage(*data))); - parent.invoke(&RasterTile::onParsed, std::move(bucket)); + parent.invoke(&RasterTile::onParsed, std::move(bucket), correlationID); } catch (...) { - parent.invoke(&RasterTile::onError, std::current_exception()); + parent.invoke(&RasterTile::onError, std::current_exception(), correlationID); } } diff --git a/src/mbgl/tile/raster_tile_worker.hpp b/src/mbgl/tile/raster_tile_worker.hpp index 44bc37ca5d..520973c3c3 100644 --- a/src/mbgl/tile/raster_tile_worker.hpp +++ b/src/mbgl/tile/raster_tile_worker.hpp @@ -13,7 +13,7 @@ class RasterTileWorker { public: RasterTileWorker(ActorRef, ActorRef); - void parse(std::shared_ptr data); + void parse(std::shared_ptr data, uint64_t correlationID); private: ActorRef parent; diff --git a/test/tile/annotation_tile.test.cpp b/test/tile/annotation_tile.test.cpp index 4d71c5b0b4..ac3e064c50 100644 --- a/test/tile/annotation_tile.test.cpp +++ b/test/tile/annotation_tile.test.cpp @@ -52,8 +52,7 @@ TEST(AnnotationTile, Issue8289) { {}, std::make_unique(), std::move(data), - 0 - }); + }, 0); auto collisionTile = std::make_unique(PlacementConfig()); @@ -65,16 +64,14 @@ TEST(AnnotationTile, Issue8289) { tile.onPlacement(GeometryTile::PlacementResult { {}, std::move(collisionTile), - 0 - }); + }, 0); // Simulate a second layout with empty data. tile.onLayout(GeometryTile::LayoutResult { {}, std::make_unique(), std::make_unique(), - 0 - }); + }, 0); std::unordered_map> result; GeometryCoordinates queryGeometry {{ Point(0, 0) }}; diff --git a/test/tile/geojson_tile.test.cpp b/test/tile/geojson_tile.test.cpp index 80e215a98d..18d11c2c14 100644 --- a/test/tile/geojson_tile.test.cpp +++ b/test/tile/geojson_tile.test.cpp @@ -76,7 +76,7 @@ TEST(GeoJSONTile, Issue7648) { TEST(GeoJSONTile, Issue9927) { GeoJSONTileTest test; - CircleLayer layer("circle", "source"); + test.style.addLayer(std::make_unique("circle", "source")); mapbox::geometry::feature_collection features; features.push_back(mapbox::geometry::feature { @@ -85,7 +85,6 @@ TEST(GeoJSONTile, Issue9927) { GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, features); - tile.setLayers({{ layer.baseImpl }}); tile.setPlacementConfig({}); while (!tile.isComplete()) { @@ -93,17 +92,18 @@ TEST(GeoJSONTile, Issue9927) { } ASSERT_TRUE(tile.isRenderable()); - ASSERT_NE(nullptr, tile.getBucket(*layer.baseImpl)); + ASSERT_NE(nullptr, tile.getBucket(*test.style.getRenderLayer("circle"))); // Make sure that once we've had a renderable tile and then receive erroneous data, we retain // the previously rendered data and keep the tile renderable. tile.setError(std::make_exception_ptr(std::runtime_error("Connection offline"))); ASSERT_TRUE(tile.isRenderable()); - ASSERT_NE(nullptr, tile.getBucket(*layer.baseImpl)); + ASSERT_NE(nullptr, tile.getBucket(*test.style.getRenderLayer("circle"))); // Then simulate a parsing failure and make sure that we keep it renderable in this situation - // as well. - tile.onError(std::make_exception_ptr(std::runtime_error("Parse error"))); + // as well. We're using 3 as a correlationID since we've done two three calls that increment + // this counter (as part of the GeoJSONTile constructor, setLayers, and setPlacementConfig). + tile.onError(std::make_exception_ptr(std::runtime_error("Parse error")), 3); ASSERT_TRUE(tile.isRenderable()); - ASSERT_NE(nullptr, tile.getBucket(*layer.baseImpl)); + ASSERT_NE(nullptr, tile.getBucket(*test.style.getRenderLayer("circle"))); } diff --git a/test/tile/raster_tile.test.cpp b/test/tile/raster_tile.test.cpp index 596f97884c..a606066e9b 100644 --- a/test/tile/raster_tile.test.cpp +++ b/test/tile/raster_tile.test.cpp @@ -47,7 +47,7 @@ TEST(RasterTile, setError) { TEST(RasterTile, onError) { RasterTileTest test; RasterTile tile(OverscaledTileID(0, 0, 0), test.tileParameters, test.tileset); - tile.onError(std::make_exception_ptr(std::runtime_error("test"))); + tile.onError(std::make_exception_ptr(std::runtime_error("test")), 0); EXPECT_FALSE(tile.isRenderable()); EXPECT_TRUE(tile.isLoaded()); EXPECT_TRUE(tile.isComplete()); @@ -56,7 +56,7 @@ TEST(RasterTile, onError) { TEST(RasterTile, onParsed) { RasterTileTest test; RasterTile tile(OverscaledTileID(0, 0, 0), test.tileParameters, test.tileset); - tile.onParsed(std::make_unique(UnassociatedImage{})); + tile.onParsed(std::make_unique(UnassociatedImage{}), 0); EXPECT_TRUE(tile.isRenderable()); EXPECT_TRUE(tile.isLoaded()); EXPECT_TRUE(tile.isComplete()); @@ -79,7 +79,7 @@ TEST(RasterTile, onParsed) { TEST(RasterTile, onParsedEmpty) { RasterTileTest test; RasterTile tile(OverscaledTileID(0, 0, 0), test.tileParameters, test.tileset); - tile.onParsed(nullptr); + tile.onParsed(nullptr, 0); EXPECT_FALSE(tile.isRenderable()); EXPECT_TRUE(tile.isLoaded()); EXPECT_TRUE(tile.isComplete()); diff --git a/test/tile/vector_tile.test.cpp b/test/tile/vector_tile.test.cpp index 37bfe8512d..336076ecc2 100644 --- a/test/tile/vector_tile.test.cpp +++ b/test/tile/vector_tile.test.cpp @@ -53,7 +53,8 @@ TEST(VectorTile, setError) { TEST(VectorTile, onError) { VectorTileTest test; VectorTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, test.tileset); - tile.onError(std::make_exception_ptr(std::runtime_error("test"))); + tile.onError(std::make_exception_ptr(std::runtime_error("test")), 0); + EXPECT_FALSE(tile.isRenderable()); EXPECT_TRUE(tile.isLoaded()); EXPECT_TRUE(tile.isComplete()); @@ -78,16 +79,14 @@ TEST(VectorTile, Issue7615) { symbolBucket }}, nullptr, - 0 - }); + }, 0); // Subsequent onLayout should not cause the existing symbol bucket to be discarded. tile.onLayout(GeometryTile::LayoutResult { {}, nullptr, nullptr, - 0 - }); + }, 0); EXPECT_EQ(symbolBucket.get(), tile.getBucket(*symbolLayer.baseImpl->createRenderLayer())); } -- cgit v1.2.1