[core] Don't provide multiple responses with the same data for 304 replies

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

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]));

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<std::string>();
             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<std::string>();
             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();
         }

test/storage/main_resource_loader.test.cpp

@@ -8,6 +8,7 @@
 #include <mbgl/storage/resource_transform.hpp>
 #include <mbgl/test/util.hpp>
 #include <mbgl/util/run_loop.hpp>
+#include <mbgl/util/timer.hpp>
 
 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<std::string>("data");
+    response.etag.emplace("snowfall");
+
+    std::unique_ptr<AsyncRequest> 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();
+}