From 0066637e58ab47c7c1c9dcec123ff850ae33f8fc Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 13 Jan 2016 16:55:44 -0800 Subject: [core] Let SQLiteCache decide when store or refresh responses --- platform/default/online_file_source.cpp | 11 ++--------- platform/default/sqlite_cache.cpp | 16 +++++++++------- src/mbgl/storage/sqlite_cache.hpp | 3 +-- test/storage/cache_response.cpp | 6 +++--- test/storage/cache_size.cpp | 5 +++-- 5 files changed, 18 insertions(+), 23 deletions(-) diff --git a/platform/default/online_file_source.cpp b/platform/default/online_file_source.cpp index 657871b907..a470a83462 100644 --- a/platform/default/online_file_source.cpp +++ b/platform/default/online_file_source.cpp @@ -244,15 +244,8 @@ void OnlineFileRequestImpl::scheduleRealRequest(OnlineFileSource::Impl& impl, bo realRequest = impl.httpContext->createRequest(resource.url, [this, &impl](std::shared_ptr response_) { realRequest = nullptr; - // Only update the cache for successful or 404 responses. - // In particular, we don't want to write a Canceled request, or one that failed due to - // connection errors to the cache. Server errors are hopefully also temporary, so we're not - // caching them either. - if (impl.cache && - (!response_->error || (response_->error->reason == Response::Error::Reason::NotFound))) { - // Store response in database. Make sure we only refresh the expires column if the data - // didn't change. - impl.cache->put(resource, *response_, response_->notModified ? SQLiteCache::Hint::Refresh : SQLiteCache::Hint::Full); + if (impl.cache) { + impl.cache->put(resource, *response_); } response = response_; diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index 53f2c10c23..444637b9af 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -367,14 +367,16 @@ void SQLiteCache::Impl::get(const Resource &resource, Callback callback) { } } -void SQLiteCache::put(const Resource &resource, const Response& response, Hint hint) { - // Can be called from any thread, but most likely from the file source thread. We are either - // storing a new response or updating the currently stored response, potentially setting a new - // expiry date. - if (hint == Hint::Full) { - thread->invoke(&Impl::put, resource, response); - } else if (hint == Hint::Refresh) { +void SQLiteCache::put(const Resource& resource, const Response& response) { + // Except for 404s, don't store errors in the cache. + if (response.error && response.error->reason != Response::Error::Reason::NotFound) { + return; + } + + if (response.notModified) { thread->invoke(&Impl::refresh, resource, response.expires); + } else { + thread->invoke(&Impl::put, resource, response); } } diff --git a/src/mbgl/storage/sqlite_cache.hpp b/src/mbgl/storage/sqlite_cache.hpp index 76e7aed440..94eeee3d46 100644 --- a/src/mbgl/storage/sqlite_cache.hpp +++ b/src/mbgl/storage/sqlite_cache.hpp @@ -26,11 +26,10 @@ public: void setMaximumCacheSize(uint64_t size); void setMaximumCacheEntrySize(uint64_t size); - enum class Hint : bool { Full, Refresh }; using Callback = std::function)>; std::unique_ptr get(const Resource&, Callback); - void put(const Resource&, const Response&, Hint hint); + void put(const Resource&, const Response&); class Impl; diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp index df895ab7b4..26f439ea8f 100644 --- a/test/storage/cache_response.cpp +++ b/test/storage/cache_response.cpp @@ -66,7 +66,7 @@ TEST_F(Storage, CacheNotFound) { // Insert existing data into the cache that will be marked as stale. Response response; response.data = std::make_shared("existing data"); - cache.put(resource, response, SQLiteCache::Hint::Full); + cache.put(resource, response); std::unique_ptr req1; std::unique_ptr req2; @@ -124,7 +124,7 @@ TEST_F(Storage, DontCacheConnectionErrors) { // Insert existing data into the cache that will be marked as stale. Response response; response.data = std::make_shared("existing data"); - cache.put(resource, response, SQLiteCache::Hint::Full); + cache.put(resource, response); std::unique_ptr req1; std::unique_ptr req2; @@ -180,7 +180,7 @@ TEST_F(Storage, DontCacheServerErrors) { // Insert existing data into the cache that will be marked as stale. Response response; response.data = std::make_shared("existing data"); - cache.put(resource, response, SQLiteCache::Hint::Full); + cache.put(resource, response); std::unique_ptr req1; std::unique_ptr req2; diff --git a/test/storage/cache_size.cpp b/test/storage/cache_size.cpp index 7cea572139..4ef4371e98 100644 --- a/test/storage/cache_size.cpp +++ b/test/storage/cache_size.cpp @@ -53,7 +53,7 @@ void insertTile(mbgl::SQLiteCache* cache, unsigned id, uint64_t size) { response.data = data; Resource resource{ Resource::Kind::Tile, url }; - cache->put(resource, response, SQLiteCache::Hint::Full); + cache->put(resource, response); } void refreshTile(mbgl::SQLiteCache* cache, unsigned id) { @@ -64,9 +64,10 @@ void refreshTile(mbgl::SQLiteCache* cache, unsigned id) { Response response; response.modified = toSeconds(SystemClock::now()); response.expires = toSeconds(SystemClock::now()) + Seconds(5000); + response.notModified = true; Resource resource{ Resource::Kind::Tile, url }; - cache->put(resource, response, SQLiteCache::Hint::Refresh); + cache->put(resource, response); } uint64_t cacheSize(mbgl::SQLiteCache* cache, unsigned entryCount, uint64_t entrySize) { -- cgit v1.2.1