diff options
-rw-r--r-- | include/mbgl/storage/online_file_source.hpp | 2 | ||||
-rw-r--r-- | platform/default/online_file_source.cpp | 127 | ||||
-rw-r--r-- | test/storage/cache_revalidate.cpp | 2 | ||||
-rw-r--r-- | test/storage/http_coalescing.cpp | 158 | ||||
-rw-r--r-- | test/test.gypi | 1 |
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', |