diff --git a/CHANGELOG.md b/CHANGELOG.md index bad7958a100..cf0a476107b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,16 +26,18 @@ - [core] Add support for using `within expression` with layout propery. ([#16194](https://github.com/mapbox/mapbox-gl-native/pull/16194)) +### 🐞 Bug fixes + +- [core] Don't provide multiple responses with the same data for 304 replies ([#16200](https://github.com/mapbox/mapbox-gl-native/pull/16200)) + + In cases when cached resource is useable, yet don't have an expiration timestamp, we provided data to the requester from the cache and the same data was returned once 304 response was received from the network. + ### 🏁 Performance improvements - [core] Loading images to style optimization ([#16187](https://github.com/mapbox/mapbox-gl-native/pull/16187)) This change enables attaching images to the style with batches and avoids massive re-allocations. Thus, it improves UI performance especially at start-up time. - - [core] Fix excessive onSpriteLoaded() notifications ([#16196](https://github.com/mapbox/mapbox-gl-native/pull/16196)) - - The excessive `onSpriteLoaded()` notifications affected the render orchestration logic and could have significant negative performance impact. - ### 🧩 Architectural changes ##### ⚠️ Breaking changes diff --git a/platform/default/src/mbgl/storage/main_resource_loader.cpp b/platform/default/src/mbgl/storage/main_resource_loader.cpp index 9bdf09fa10f..aa8d745170a 100644 --- a/platform/default/src/mbgl/storage/main_resource_loader.cpp +++ b/platform/default/src/mbgl/storage/main_resource_loader.cpp @@ -81,13 +81,17 @@ class MainResourceLoaderThread { callback(response); // Set the priority of existing resource to low if it's expired but usable. res.setPriority(Resource::Priority::Low); + } else { + // Set prior data only if it was not returned to the requester. + // Once we get 304 response from the network, we will forward response + // to the requester. + res.priorData = response.data; } // Copy response fields for cache control request res.priorModified = response.modified; res.priorExpires = response.expires; res.priorEtag = response.etag; - res.priorData = response.data; } tasks[req] = requestFromNetwork(res, std::move(tasks[req])); diff --git a/src/mbgl/sprite/sprite_loader.cpp b/src/mbgl/sprite/sprite_loader.cpp index 6051de7ef4e..60bab2bf6eb 100644 --- a/src/mbgl/sprite/sprite_loader.cpp +++ b/src/mbgl/sprite/sprite_loader.cpp @@ -58,8 +58,9 @@ void SpriteLoader::load(const std::string& url, FileSource& fileSource) { } else if (res.noContent) { loader->json = std::make_shared(); emitSpriteLoadedIfComplete(); - } else if (loader->json != res.data) { // They can be equal, see OnlineFileRequest::completed(). + } else { // Only trigger a sprite loaded event we got new data. + assert(loader->json != res.data); loader->json = std::move(res.data); emitSpriteLoadedIfComplete(); } @@ -73,7 +74,8 @@ void SpriteLoader::load(const std::string& url, FileSource& fileSource) { } else if (res.noContent) { loader->image = std::make_shared(); emitSpriteLoadedIfComplete(); - } else if (loader->image != res.data) { // They can be equal - see OnlineFileRequest::completed(). + } else { + assert(loader->image != res.data); loader->image = std::move(res.data); emitSpriteLoadedIfComplete(); } diff --git a/test/storage/main_resource_loader.test.cpp b/test/storage/main_resource_loader.test.cpp index ee9211b0645..b5245dbad89 100644 --- a/test/storage/main_resource_loader.test.cpp +++ b/test/storage/main_resource_loader.test.cpp @@ -8,6 +8,7 @@ #include #include #include +#include using namespace mbgl; @@ -749,3 +750,38 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(CachedResourceLowPriority)) { loop.run(); } + +TEST(MainResourceLoader, TEST_REQUIRES_SERVER(NoDoubleDispatch)) { + util::RunLoop loop; + MainResourceLoader fs(ResourceOptions{}); + + const Resource resource{Resource::Unknown, "http://127.0.0.1:3000/revalidate-same"}; + Response response; + response.data = std::make_shared("data"); + response.etag.emplace("snowfall"); + + std::unique_ptr req; + unsigned responseCount = 0u; + auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + dbfs->forward(resource, response, [&] { + req = fs.request(resource, [&](Response res) { + EXPECT_EQ(nullptr, res.error); + EXPECT_FALSE(bool(res.modified)); + EXPECT_TRUE(bool(res.etag)); + EXPECT_EQ("snowfall", *res.etag); + if (!res.notModified) { + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("data", *res.data); + ++responseCount; + } + }); + }); + + util::Timer timer; + timer.start(Milliseconds(100), Duration::zero(), [&loop, &responseCount] { + EXPECT_EQ(1u, responseCount); + loop.stop(); + }); + + loop.run(); +}