[core] Remove request coalescing in OnlineFileSource

5 files changed, 26 insertions, 264 deletions

diff --git a/include/mbgl/storage/online_file_source.hpp b/include/mbgl/storage/online_file_source.hpp
index 1725af844d..e5253bc3fe 100644
--- a/include/mbgl/storage/online_file_source.hpp
+++ b/include/mbgl/storage/online_file_source.hpp
@@ -24,7 +24,7 @@ private:
     friend class OnlineFileRequest;
     friend class OnlineFileRequestImpl;
 
-    void cancel(const Resource&, FileRequest*);
+    void cancel(FileRequest*);
 
     class Impl;
     const std::unique_ptr<util::Thread<Impl>> thread;
diff --git a/platform/default/online_file_source.cpp b/platform/default/online_file_source.cpp
index 5c987a3edd..330ecba091 100644
--- a/platform/default/online_file_source.cpp
+++ b/platform/default/online_file_source.cpp
@@ -24,19 +24,15 @@ class RequestBase;
 
 class OnlineFileRequest : public FileRequest {
 public:
-    OnlineFileRequest(const Resource& resource_,
-                       OnlineFileSource& fileSource_)
-        : resource(resource_),
-          fileSource(fileSource_) {
+    OnlineFileRequest(OnlineFileSource& fileSource_)
+        : fileSource(fileSource_) {
     }
 
     ~OnlineFileRequest() {
-        fileSource.cancel(resource, this);
+        fileSource.cancel(this);
     }
 
-    Resource resource;
     OnlineFileSource& fileSource;
-
     std::unique_ptr<WorkRequest> workRequest;
 };
 
@@ -44,14 +40,9 @@ class OnlineFileRequestImpl : public util::noncopyable {
 public:
     using Callback = std::function<void (Response)>;
 
-    OnlineFileRequestImpl(const Resource&);
+    OnlineFileRequestImpl(const Resource&, Callback, OnlineFileSource::Impl&);
     ~OnlineFileRequestImpl();
 
-    // Observer accessors.
-    void addObserver(FileRequest*, Callback, OnlineFileSource::Impl&);
-    void removeObserver(FileRequest*);
-    bool hasObservers() const;
-
     void networkIsReachableAgain(OnlineFileSource::Impl&);
 
 private:
@@ -62,13 +53,10 @@ private:
     std::unique_ptr<WorkRequest> cacheRequest;
     RequestBase* realRequest = nullptr;
     util::Timer realRequestTimer;
+    Callback callback;
 
-    // Stores a set of all observing Request objects.
-    std::unordered_map<FileRequest*, Callback> observers;
-
-    // The current response data. We're storing it because we can satisfy requests for the same
-    // resource directly by returning this response object. We also need it to create conditional
-    // HTTP requests, and to check whether new responses we got changed any data.
+    // The current response data. Used to create conditional HTTP requests, and to check whether
+    // new responses we got changed any data.
     std::shared_ptr<const Response> response;
 
     // Counts the number of subsequent failed requests. We're using this value for exponential
@@ -86,12 +74,12 @@ public:
     void networkIsReachableAgain();
 
     void add(Resource, FileRequest*, Callback);
-    void cancel(Resource, FileRequest*);
+    void cancel(FileRequest*);
 
 private:
     friend OnlineFileRequestImpl;
 
-    std::unordered_map<Resource, std::unique_ptr<OnlineFileRequestImpl>, Resource::Hash> pending;
+    std::unordered_map<FileRequest*, std::unique_ptr<OnlineFileRequestImpl>> pending;
     FileCache* const cache;
     const std::unique_ptr<HTTPContextBase> httpContext;
     util::AsyncTask reachability;
@@ -135,13 +123,13 @@ std::unique_ptr<FileRequest> OnlineFileSource::request(const Resource& resource,
     }
 
     Resource res { resource.kind, url };
-    auto req = std::make_unique<OnlineFileRequest>(res, *this);
+    auto req = std::make_unique<OnlineFileRequest>(*this);
     req->workRequest = thread->invokeWithCallback(&Impl::add, callback, res, req.get());
     return std::move(req);
 }
 
-void OnlineFileSource::cancel(const Resource& res, FileRequest* req) {
-    thread->invoke(&Impl::cancel, res, req);
+void OnlineFileSource::cancel(FileRequest* req) {
+    thread->invoke(&Impl::cancel, req);
 }
 
 // ----- Impl -----
@@ -167,34 +155,23 @@ void OnlineFileSource::Impl::networkIsReachableAgain() {
 }
 
 void OnlineFileSource::Impl::add(Resource resource, FileRequest* req, Callback callback) {
-    auto& request = *pending.emplace(resource,
-        std::make_unique<OnlineFileRequestImpl>(resource)).first->second;
-
-    // Add this request as an observer so that it'll get notified when something about this
-    // request changes.
-    request.addObserver(req, callback, *this);
+    pending[req] = std::make_unique<OnlineFileRequestImpl>(resource, callback, *this);
 }
 
-void OnlineFileSource::Impl::cancel(Resource resource, FileRequest* req) {
-    auto it = pending.find(resource);
-    if (it != pending.end()) {
-        // If the number of dependent requests of the OnlineFileRequest drops to zero,
-        // cancel the request and remove it from the pending list.
-        auto& request = *it->second;
-        request.removeObserver(req);
-        if (!request.hasObservers()) {
-            pending.erase(it);
-        }
-    } else {
-        // There is no request for this URL anymore. Likely, the request already completed
-        // before we got around to process the cancelation request.
-    }
+void OnlineFileSource::Impl::cancel(FileRequest* req) {
+    pending.erase(req);
 }
 
 // ----- OnlineFileRequest -----
 
-OnlineFileRequestImpl::OnlineFileRequestImpl(const Resource& resource_)
-    : resource(resource_) {
+OnlineFileRequestImpl::OnlineFileRequestImpl(const Resource& resource_, Callback callback_, OnlineFileSource::Impl& impl)
+    : resource(resource_),
+      callback(callback_) {
+    if (impl.cache) {
+        scheduleCacheRequest(impl);
+    } else {
+        scheduleRealRequest(impl);
+    }
 }
 
 OnlineFileRequestImpl::~OnlineFileRequestImpl() {
@@ -205,55 +182,6 @@ OnlineFileRequestImpl::~OnlineFileRequestImpl() {
     // realRequestTimer and cacheRequest are automatically canceled upon destruction.
 }
 
-void OnlineFileRequestImpl::addObserver(FileRequest* req, Callback callback, OnlineFileSource::Impl& impl) {
-    if (response) {
-        // We've at least obtained a cache value, potentially we also got a final response.
-        // Before returning the existing response, update the `stale` flag if necessary.
-        if (!response->stale && response->isExpired()) {
-            // Create a new Response object with `stale = true`, but the same data, and
-            // replace the current request object we have.
-            // We're not immediately swapping the member variable because it's declared as `const`, and
-            // we first have to update the `stale` flag.
-            auto staleResponse = std::make_shared<Response>(*response);
-            staleResponse->stale = true;
-            response = staleResponse;
-        }
-
-        callback(*response);
-
-        if (response->stale && !realRequest) {
-            // We've returned a stale response; now make sure the requester also gets a fresh
-            // response eventually. It's possible that there's already a request in progress.
-            // Note that this will also trigger updates to all other existing listeners.
-            // Since we already have data, we're going to verify
-            scheduleRealRequest(impl);
-        } else {
-            // The response is still fresh (or there's already a request for refreshing the resource
-            // in progress), so there's nothing we need to do.
-        }
-    } else if (!cacheRequest && !realRequest) {
-        // There is no request in progress, and we don't have a response yet. This means we'll have
-        // to start the request ourselves.
-        if (impl.cache) {
-            scheduleCacheRequest(impl);
-        } else {
-            scheduleRealRequest(impl);
-        }
-    } else {
-        // There is a request in progress. We just have to wait.
-    }
-
-    observers.emplace(req, callback);
-}
-
-void OnlineFileRequestImpl::removeObserver(FileRequest* req) {
-    observers.erase(req);
-}
-
-bool OnlineFileRequestImpl::hasObservers() const {
-    return !observers.empty();
-}
-
 void OnlineFileRequestImpl::scheduleCacheRequest(OnlineFileSource::Impl& impl) {
     // Check the cache for existing data so that we can potentially
     // revalidate the information without having to redownload everything.
@@ -263,9 +191,7 @@ void OnlineFileRequestImpl::scheduleCacheRequest(OnlineFileSource::Impl& impl) {
         if (response_) {
             response_->stale = response_->isExpired();
             response = response_;
-            for (auto& req : observers) {
-                req.second(*response);
-            }
+            callback(*response);
         }
 
         scheduleRealRequest(impl);
@@ -341,10 +267,7 @@ void OnlineFileRequestImpl::scheduleRealRequest(OnlineFileSource::Impl& impl, bo
                 failedRequests = 0;
             }
 
-            for (auto& req : observers) {
-                req.second(*response);
-            }
-
+            callback(*response);
             scheduleRealRequest(impl);
         }, response);
     });
diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp
index e0e0672ad5..ffc622f37c 100644
--- a/test/storage/cache_revalidate.cpp
+++ b/test/storage/cache_revalidate.cpp
@@ -49,7 +49,6 @@ TEST_F(Storage, CacheRevalidateSame) {
             EXPECT_EQ(nullptr, res2.error);
             EXPECT_EQ(false, res2.stale);
             ASSERT_TRUE(res2.data.get());
-            EXPECT_EQ(res.data, res2.data);
             EXPECT_EQ("Response", *res2.data);
             // We use this to indicate that a 304 reply came back.
             EXPECT_LT(Seconds::zero(), res2.expires);
@@ -111,7 +110,6 @@ TEST_F(Storage, CacheRevalidateModified) {
             EXPECT_EQ(false, res2.stale);
             ASSERT_TRUE(res2.data.get());
             EXPECT_EQ("Response", *res2.data);
-            EXPECT_EQ(res.data, res2.data);
             // We use this to indicate that a 304 reply came back.
             EXPECT_LT(Seconds::zero(), res2.expires);
             EXPECT_EQ(1420070400, res2.modified.count());
diff --git a/test/storage/http_coalescing.cpp b/test/storage/http_coalescing.cpp
deleted file mode 100644
index dc4d993b35..0000000000
--- a/test/storage/http_coalescing.cpp
+++ /dev/null
@@ -1,158 +0,0 @@
-#include "storage.hpp"
-
-#include <mbgl/storage/online_file_source.hpp>
-#include <mbgl/util/chrono.hpp>
-#include <mbgl/util/run_loop.hpp>
-
-TEST_F(Storage, HTTPCoalescing) {
-    SCOPED_TEST(HTTPCoalescing)
-
-    static int counter = 0;
-    const static int total = 4;
-
-    using namespace mbgl;
-
-    util::RunLoop loop;
-    OnlineFileSource fs(nullptr);
-
-    static const Response *reference = nullptr;
-
-    const auto complete = [&](const Response &res) {
-        counter++;
-
-        // Make sure all of the Response objects are the same.
-        if (!reference) {
-            reference = &res;
-        } else {
-            EXPECT_EQ(&res, reference);
-        }
-
-        EXPECT_EQ(nullptr, res.error);
-        ASSERT_TRUE(res.data.get());
-        EXPECT_EQ("Hello World!", *res.data);
-        EXPECT_EQ(Seconds::zero(), res.expires);
-        EXPECT_EQ(Seconds::zero(), res.modified);
-        EXPECT_EQ("", res.etag);
-
-        if (counter >= total) {
-            loop.stop();
-            HTTPCoalescing.finish();
-        }
-    };
-
-    const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" };
-
-    std::unique_ptr<FileRequest> reqs[total];
-    for (int i = 0; i < total; i++) {
-        reqs[i] = fs.request(resource, [&complete, &fs, &reqs, i] (Response res) {
-            reqs[i].reset();
-            complete(res);
-        });
-    }
-
-    loop.run();
-}
-
-TEST_F(Storage, HTTPMultiple) {
-    SCOPED_TEST(HTTPMultiple)
-
-    using namespace mbgl;
-
-    util::RunLoop loop;
-    OnlineFileSource fs(nullptr);
-
-    const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test?expires=2147483647" };
-    std::unique_ptr<FileRequest> req1;
-    std::unique_ptr<FileRequest> req2;
-    req1 = fs.request(resource, [&] (Response res) {
-        // Do not cancel the request right away.
-        EXPECT_EQ(nullptr, res.error);
-        ASSERT_TRUE(res.data.get());
-        EXPECT_EQ("Hello World!", *res.data);
-        EXPECT_EQ(2147483647, res.expires.count());
-        EXPECT_EQ(Seconds::zero(), res.modified);
-        EXPECT_EQ("", res.etag);
-
-        // Start a second request for the same resource after the first one has been completed.
-        req2 = fs.request(resource, [&, res] (Response res2) {
-            // Make sure we get the same data as before.
-            EXPECT_EQ(res.data.get(), res2.data.get());
-
-            // Now cancel both requests after both have been notified.
-            req1.reset();
-            req2.reset();
-
-            EXPECT_EQ(nullptr, res2.error);
-            ASSERT_TRUE(res2.data.get());
-            EXPECT_EQ("Hello World!", *res2.data);
-            EXPECT_EQ(2147483647, res2.expires.count());
-            EXPECT_EQ(Seconds::zero(), res2.modified);
-            EXPECT_EQ("", res2.etag);
-
-            loop.stop();
-            HTTPMultiple.finish();
-        });
-    });
-
-    loop.run();
-}
-
-// Tests that we get stale responses from previous requests when requesting the same thing again.
-TEST_F(Storage, HTTPStale) {
-    SCOPED_TEST(HTTPStale)
-
-    using namespace mbgl;
-
-    util::RunLoop loop;
-    OnlineFileSource fs(nullptr);
-
-    int updates = 0;
-    int stale = 0;
-
-    const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" };
-    std::unique_ptr<FileRequest> req1;
-    std::unique_ptr<FileRequest> req2;
-    req1 = fs.request(resource, [&] (Response res) {
-        // Do not cancel the request right away.
-        EXPECT_EQ(nullptr, res.error);
-        ASSERT_TRUE(res.data.get());
-        EXPECT_EQ("Hello World!", *res.data);
-        EXPECT_EQ(false, res.stale);
-        EXPECT_EQ(Seconds::zero(), res.expires);
-        EXPECT_EQ(Seconds::zero(), res.modified);
-        EXPECT_EQ("", res.etag);
-
-        // Don't start the request twice in case this callback gets fired multiple times.
-        if (req2) {
-            return;
-        }
-
-        updates++;
-
-        // Start a second request for the same resource after the first one has been completed.
-        req2 = fs.request(resource, [&] (Response res2) {
-            EXPECT_EQ(nullptr, res2.error);
-            ASSERT_TRUE(res2.data.get());
-            EXPECT_EQ("Hello World!", *res2.data);
-            EXPECT_EQ(Seconds::zero(), res2.expires);
-            EXPECT_EQ(Seconds::zero(), res2.modified);
-            EXPECT_EQ("", res2.etag);
-
-            if (res2.stale) {
-                EXPECT_EQ(0, stale);
-                stale++;
-            } else {
-                // Now cancel both requests after both have been notified.
-                req1.reset();
-                req2.reset();
-                loop.stop();
-                HTTPStale.finish();
-            }
-        });
-    });
-
-    loop.run();
-
-    EXPECT_EQ(1, stale);
-    EXPECT_EQ(1, updates);
-}
diff --git a/test/test.gypi b/test/test.gypi
index e92a706894..f8818e2084 100644
--- a/test/test.gypi
+++ b/test/test.gypi
@@ -76,7 +76,6 @@
         'storage/asset_file_source.cpp',
         'storage/headers.cpp',
         'storage/http_cancel.cpp',
-        'storage/http_coalescing.cpp',
         'storage/http_error.cpp',
         'storage/http_header_parsing.cpp',
         'storage/http_issue_1369.cpp',