[core] Include prior values of caching headers in Resource

This allows the FileSource interface itself to support revalidation. We could (and probably should) now rewrite HTTPContextBase implementations as FileSource implementations.

10 files changed, 130 insertions, 188 deletions

diff --git a/include/mbgl/storage/resource.hpp b/include/mbgl/storage/resource.hpp
index 1bbd90ea37..8a61702183 100644
--- a/include/mbgl/storage/resource.hpp
+++ b/include/mbgl/storage/resource.hpp
@@ -1,12 +1,15 @@
 #ifndef MBGL_STORAGE_RESOURCE
 #define MBGL_STORAGE_RESOURCE
 
+#include <mbgl/storage/response.hpp>
+#include <mbgl/util/optional.hpp>
+
 #include <string>
-#include <functional>
 
 namespace mbgl {
 
-struct Resource {
+class Resource {
+public:
     enum Kind : uint8_t {
         Unknown = 0,
         Style,
@@ -17,19 +20,17 @@ struct Resource {
         SpriteJSON
     };
 
-    const Kind kind;
-    const std::string url;
-
-    inline bool operator==(const Resource &res) const {
-        return kind == res.kind && url == res.url;
+    Resource(Kind kind_, const std::string& url_)
+        : kind(kind_),
+          url(url_) {
     }
 
-    struct Hash {
-        std::size_t operator()(Resource const& r) const {
-            return std::hash<std::string>()(r.url) ^ (std::hash<uint8_t>()(r.kind) << 1);
-        }
-    };
+    const Kind kind;
+    const std::string url;
 
+    optional<SystemTimePoint> priorModified;
+    optional<SystemTimePoint> priorExpires;
+    optional<std::string> priorEtag;
 };
 
 } // namespace mbgl
diff --git a/include/mbgl/storage/response.hpp b/include/mbgl/storage/response.hpp
index e81af710f7..29af5281fe 100644
--- a/include/mbgl/storage/response.hpp
+++ b/include/mbgl/storage/response.hpp
@@ -23,7 +23,7 @@ public:
     // This is set to true for 304 Not Modified responses.
     bool notModified = false;
 
-    // The actual data of the response. This is guaranteed to never be empty.
+    // The actual data of the response. This is null if notModified is true.
     std::shared_ptr<const std::string> data;
 
     optional<SystemTimePoint> modified;
@@ -34,7 +34,7 @@ public:
 class Response::Error {
 public:
     enum class Reason : uint8_t {
-        // Success = 1, // Reserve 1 for Success.
+        Success = 1,
         NotFound = 2,
         Server = 3,
         Connection = 4,
diff --git a/platform/android/src/http_request_android.cpp b/platform/android/src/http_request_android.cpp
index 7a234a609c..9db7416906 100644
--- a/platform/android/src/http_request_android.cpp
+++ b/platform/android/src/http_request_android.cpp
@@ -1,5 +1,6 @@
 #include <mbgl/storage/http_context_base.hpp>
 #include <mbgl/storage/http_request_base.hpp>
+#include <mbgl/storage/resource.hpp>
 #include <mbgl/storage/response.hpp>
 #include <mbgl/platform/log.hpp>
 #include "jni.hpp"
@@ -22,9 +23,7 @@ public:
     explicit HTTPAndroidContext();
     ~HTTPAndroidContext();
 
-    HTTPRequestBase* createRequest(const std::string& url,
-                               HTTPRequestBase::Callback,
-                               std::shared_ptr<const Response>) final;
+    HTTPRequestBase* createRequest(const Resource&, HTTPRequestBase::Callback) final;
 
     JavaVM *vm = nullptr;
     jobject obj = nullptr;
@@ -32,10 +31,7 @@ public:
 
 class HTTPAndroidRequest : public HTTPRequestBase {
 public:
-    HTTPAndroidRequest(HTTPAndroidContext*,
-                const std::string& url,
-                Callback,
-                std::shared_ptr<const Response>);
+    HTTPAndroidRequest(HTTPAndroidContext*, const Resource&, Callback);
     ~HTTPAndroidRequest();
 
     void cancel() final;
@@ -107,32 +103,28 @@ HTTPAndroidContext::~HTTPAndroidContext() {
     vm = nullptr;
 }
 
-HTTPRequestBase* HTTPAndroidContext::createRequest(const std::string& url,
-                                            HTTPRequestBase::Callback callback,
-                                            std::shared_ptr<const Response> response) {
-    return new HTTPAndroidRequest(this, url, callback, response);
+HTTPRequestBase* HTTPAndroidContext::createRequest(const Resource& resource, HTTPRequestBase::Callback callback) {
+    return new HTTPAndroidRequest(this, resource, callback);
 }
 
-HTTPAndroidRequest::HTTPAndroidRequest(HTTPAndroidContext* context_, const std::string& url_, Callback callback_, std::shared_ptr<const Response> response_)
-    : HTTPRequestBase(url_, callback_),
+HTTPAndroidRequest::HTTPAndroidRequest(HTTPAndroidContext* context_, const Resource& resource_, Callback callback_)
+    : HTTPRequestBase(resource_, callback_),
       context(context_),
-      existingResponse(response_),
       async([this] { finish(); }) {
 
     std::string etagStr;
     std::string modifiedStr;
-    if (existingResponse) {
-        if (existingResponse->etag) {
-            etagStr = *existingResponse->etag;
-        } else if (existingResponse->modified) {
-            modifiedStr = util::rfc1123(*existingResponse->modified);
-        }
+
+    if (resource.priorEtag) {
+        etagStr = *resource.priorEtag;
+    } else if (resource.priorModified) {
+        modifiedStr = util::rfc1123(*resource.priorModified);
     }
 
     JNIEnv *env = nullptr;
     bool detach = mbgl::android::attach_jni_thread(context->vm, &env, "HTTPAndroidContext::HTTPAndroidRequest()");
 
-    jstring resourceUrl = mbgl::android::std_string_to_jstring(env, url);
+    jstring resourceUrl = mbgl::android::std_string_to_jstring(env, resource.url);
     jstring userAgent = mbgl::android::std_string_to_jstring(env, "MapboxGL/1.0");
     jstring etag = mbgl::android::std_string_to_jstring(env, etagStr);
     jstring modified = mbgl::android::std_string_to_jstring(env, modifiedStr);
@@ -180,7 +172,7 @@ void HTTPAndroidRequest::cancel() {
 
 void HTTPAndroidRequest::finish() {
     if (!cancelled) {
-        notify(std::move(response));
+        notify(*response);
     }
 
     delete this;
@@ -216,22 +208,7 @@ void HTTPAndroidRequest::onResponse(JNIEnv* env, int code, jstring /* message */
         // Nothing to do; this is what we want
     } else if (code == 304) {
         response->notModified = true;
-
-        if (existingResponse) {
-            response->data = existingResponse->data;
-
-            if (!response->expires) {
-                response->expires = existingResponse->expires;
-            }
-
-            if (!response->modified) {
-                response->modified = existingResponse->modified;
-            }
-
-            if (!response->etag) {
-                response->etag = existingResponse->etag;
-            }
-        }
+        response->data.reset();
     } else if (code == 404) {
         response->error = std::make_unique<Error>(Error::Reason::NotFound, "HTTP status code 404");
     } else if (code >= 500 && code < 600) {
diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm
index 2421c69c66..0166564727 100644
--- a/platform/darwin/http_request_nsurl.mm
+++ b/platform/darwin/http_request_nsurl.mm
@@ -1,5 +1,6 @@
 #include <mbgl/storage/http_context_base.hpp>
 #include <mbgl/storage/http_request_base.hpp>
+#include <mbgl/storage/resource.hpp>
 #include <mbgl/storage/response.hpp>
 
 #include <mbgl/util/async_task.hpp>
@@ -16,10 +17,7 @@ class HTTPNSURLContext;
 
 class HTTPNSURLRequest : public HTTPRequestBase {
 public:
-    HTTPNSURLRequest(HTTPNSURLContext*,
-                const std::string& url,
-                Callback,
-                std::shared_ptr<const Response>);
+    HTTPNSURLRequest(HTTPNSURLContext*, const Resource&, Callback);
     ~HTTPNSURLRequest();
 
     void cancel() final;
@@ -32,7 +30,6 @@ private:
     bool cancelled = false;
     NSURLSessionDataTask *task = nullptr;
     std::unique_ptr<Response> response;
-    const std::shared_ptr<const Response> existingResponse;
     util::AsyncTask async;
 };
 
@@ -43,9 +40,7 @@ public:
     HTTPNSURLContext();
     ~HTTPNSURLContext();
 
-    HTTPRequestBase* createRequest(const std::string& url,
-                               HTTPRequestBase::Callback,
-                               std::shared_ptr<const Response>) final;
+    HTTPRequestBase* createRequest(const Resource&, HTTPRequestBase::Callback) final;
 
     NSURLSession *session = nil;
     NSString *userAgent = nil;
@@ -79,21 +74,17 @@ HTTPNSURLContext::~HTTPNSURLContext() {
     userAgent = nullptr;
 }
 
-HTTPRequestBase* HTTPNSURLContext::createRequest(const std::string& url,
-                                             HTTPRequestBase::Callback callback,
-                                             std::shared_ptr<const Response> response) {
-    return new HTTPNSURLRequest(this, url, callback, response);
+HTTPRequestBase* HTTPNSURLContext::createRequest(const Resource& resource, HTTPRequestBase::Callback callback) {
+    return new HTTPNSURLRequest(this, resource, callback);
 }
 
 // -------------------------------------------------------------------------------------------------
 
 HTTPNSURLRequest::HTTPNSURLRequest(HTTPNSURLContext* context_,
-                                   const std::string& url_,
-                                   Callback callback_,
-                                   std::shared_ptr<const Response> existingResponse_)
-    : HTTPRequestBase(url_, callback_),
+                                   const Resource& resource_,
+                                   Callback callback_)
+    : HTTPRequestBase(resource_, callback_),
       context(context_),
-      existingResponse(existingResponse_),
       async([this] { handleResponse(); }) {
     // Hold the main loop alive until the request returns. This
     // is needed because completion handler runs in another
@@ -101,7 +92,7 @@ HTTPNSURLRequest::HTTPNSURLRequest(HTTPNSURLContext* context_,
     util::RunLoop::Get()->ref();
 
     @autoreleasepool {
-        NSURL* url = [NSURL URLWithString:@(url_.c_str())];
+        NSURL* url = [NSURL URLWithString:@(resource.url.c_str())];
         if (context->accountType == 0 &&
             ([url.host isEqualToString:@"mapbox.com"] || [url.host hasSuffix:@".mapbox.com"])) {
             NSString* absoluteString = [url.absoluteString
@@ -110,14 +101,12 @@ HTTPNSURLRequest::HTTPNSURLRequest(HTTPNSURLContext* context_,
         }
 
         NSMutableURLRequest* req = [NSMutableURLRequest requestWithURL:url];
-        if (existingResponse) {
-            if (existingResponse->etag) {
-                [req addValue:@((*existingResponse->etag).c_str())
-                     forHTTPHeaderField:@"If-None-Match"];
-            } else if (existingResponse->modified) {
-                [req addValue:@(util::rfc1123(*existingResponse->modified).c_str())
-                     forHTTPHeaderField:@"If-Modified-Since"];
-            }
+        if (resource.priorEtag) {
+            [req addValue:@(resource.priorEtag->c_str())
+                 forHTTPHeaderField:@"If-None-Match"];
+        } else if (resource.priorModified) {
+            [req addValue:@(util::rfc1123(*resource.priorModified).c_str())
+                 forHTTPHeaderField:@"If-Modified-Since"];
         }
 
         [req addValue:context->userAgent forHTTPHeaderField:@"User-Agent"];
@@ -145,7 +134,7 @@ void HTTPNSURLRequest::handleResponse() {
 
     if (!cancelled) {
         // Actually return the response.
-        notify(std::move(response));
+        notify(*response);
     }
 
     delete this;
@@ -232,22 +221,7 @@ void HTTPNSURLRequest::handleResult(NSData *data, NSURLResponse *res, NSError *e
             // Nothing to do; this is what we want.
         } else if (responseCode == 304) {
             response->notModified = true;
-
-            if (existingResponse) {
-                response->data = existingResponse->data;
-
-                if (!response->expires) {
-                    response->expires = existingResponse->expires;
-                }
-
-                if (!response->modified) {
-                    response->modified = existingResponse->modified;
-                }
-
-                if (!response->etag) {
-                    response->etag = existingResponse->etag;
-                }
-            }
+            response->data.reset();
         } else if (responseCode == 404) {
             response->error =
                 std::make_unique<Error>(Error::Reason::NotFound, "HTTP status code 404");
diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp
index 3129dc7a94..9cc5815ad1 100644
--- a/platform/default/http_request_curl.cpp
+++ b/platform/default/http_request_curl.cpp
@@ -1,5 +1,6 @@
 #include <mbgl/storage/http_context_base.hpp>
 #include <mbgl/storage/http_request_base.hpp>
+#include <mbgl/storage/resource.hpp>
 #include <mbgl/storage/response.hpp>
 #include <mbgl/platform/log.hpp>
 
@@ -45,9 +46,7 @@ public:
     HTTPCURLContext();
     ~HTTPCURLContext();
 
-    HTTPRequestBase* createRequest(const std::string& url,
-                               HTTPRequestBase::Callback,
-                               std::shared_ptr<const Response>) final;
+    HTTPRequestBase* createRequest(const Resource&, HTTPRequestBase::Callback) final;
 
     static int handleSocket(CURL *handle, curl_socket_t s, int action, void *userp, void *socketp);
     static int startTimeout(CURLM *multi, long timeout_ms, void *userp);
@@ -77,10 +76,7 @@ class HTTPCURLRequest : public HTTPRequestBase {
     MBGL_STORE_THREAD(tid)
 
 public:
-    HTTPCURLRequest(HTTPCURLContext*,
-                const std::string& url,
-                Callback,
-                std::shared_ptr<const Response>);
+    HTTPCURLRequest(HTTPCURLContext*, const Resource&, Callback);
     ~HTTPCURLRequest();
 
     void cancel() final;
@@ -137,10 +133,8 @@ HTTPCURLContext::~HTTPCURLContext() {
     timeout.stop();
 }
 
-HTTPRequestBase* HTTPCURLContext::createRequest(const std::string& url,
-                                            HTTPRequestBase::Callback callback,
-                                            std::shared_ptr<const Response> response) {
-    return new HTTPCURLRequest(this, url, callback, response);
+HTTPRequestBase* HTTPCURLContext::createRequest(const Resource& resource, HTTPRequestBase::Callback callback) {
+    return new HTTPCURLRequest(this, resource, callback);
 }
 
 CURL *HTTPCURLContext::getHandle() {
@@ -351,25 +345,22 @@ static CURLcode sslctx_function(CURL * /* curl */, void *sslctx, void * /* parm
 }
 #endif
 
-HTTPCURLRequest::HTTPCURLRequest(HTTPCURLContext* context_, const std::string& url_, Callback callback_, std::shared_ptr<const Response> response_)
-    : HTTPRequestBase(url_, callback_),
+HTTPCURLRequest::HTTPCURLRequest(HTTPCURLContext* context_, const Resource& resource_, Callback callback_)
+    : HTTPRequestBase(resource_, callback_),
       context(context_),
-      existingResponse(response_),
       handle(context->getHandle()) {
     // Zero out the error buffer.
     memset(error, 0, sizeof(error));
 
     // If there's already a response, set the correct etags/modified headers to make sure we are
     // getting a 304 response if possible. This avoids redownloading unchanged data.
-    if (existingResponse) {
-        if (existingResponse->etag) {
-            const std::string header = std::string("If-None-Match: ") + *existingResponse->etag;
-            headers = curl_slist_append(headers, header.c_str());
-        } else if (existingResponse->modified) {
-            const std::string time =
-                std::string("If-Modified-Since: ") + util::rfc1123(*existingResponse->modified);
-            headers = curl_slist_append(headers, time.c_str());
-        }
+    if (resource.priorEtag) {
+        const std::string header = std::string("If-None-Match: ") + *resource.priorEtag;
+        headers = curl_slist_append(headers, header.c_str());
+    } else if (resource.priorModified) {
+        const std::string time =
+            std::string("If-Modified-Since: ") + util::rfc1123(*resource.priorModified);
+        headers = curl_slist_append(headers, time.c_str());
     }
 
     if (headers) {
@@ -385,7 +376,7 @@ HTTPCURLRequest::HTTPCURLRequest(HTTPCURLContext* context_, const std::string& u
     handleError(curl_easy_setopt(handle, CURLOPT_CAINFO, "ca-bundle.crt"));
 #endif
     handleError(curl_easy_setopt(handle, CURLOPT_FOLLOWLOCATION, 1));
-    handleError(curl_easy_setopt(handle, CURLOPT_URL, url.c_str()));
+    handleError(curl_easy_setopt(handle, CURLOPT_URL, resource.url.c_str()));
     handleError(curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION, writeCallback));
     handleError(curl_easy_setopt(handle, CURLOPT_WRITEDATA, this));
     handleError(curl_easy_setopt(handle, CURLOPT_HEADERFUNCTION, headerCallback));
@@ -529,22 +520,7 @@ void HTTPCURLRequest::handleResult(CURLcode code) {
             // Nothing to do; this is what we want.
         } else if (responseCode == 304) {
             response->notModified = true;
-
-            if (existingResponse) {
-                response->data = existingResponse->data;
-
-                if (!response->expires) {
-                    response->expires = existingResponse->expires;
-                }
-
-                if (!response->modified) {
-                    response->modified = existingResponse->modified;
-                }
-
-                if (!response->etag) {
-                    response->etag = existingResponse->etag;
-                }
-            }
+            response->data.reset();
         } else if (responseCode == 404) {
             response->error =
                 std::make_unique<Error>(Error::Reason::NotFound, "HTTP status code 404");
@@ -560,7 +536,7 @@ void HTTPCURLRequest::handleResult(CURLcode code) {
     }
 
     // Actually return the response.
-    notify(std::move(response));
+    notify(*response);
     delete this;
 }
 
diff --git a/platform/default/online_file_source.cpp b/platform/default/online_file_source.cpp
index 89a9d019f0..988147a373 100644
--- a/platform/default/online_file_source.cpp
+++ b/platform/default/online_file_source.cpp
@@ -48,19 +48,16 @@ private:
     void scheduleCacheRequest(OnlineFileSource::Impl&);
     void scheduleRealRequest(OnlineFileSource::Impl&, bool forceImmediate = false);
 
-    const Resource resource;
+    Resource resource;
     std::unique_ptr<WorkRequest> cacheRequest;
     HTTPRequestBase* realRequest = nullptr;
     util::Timer realRequestTimer;
     Callback callback;
 
-    // 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
     // backoff when retrying requests.
     uint32_t failedRequests = 0;
+    Response::Error::Reason failedRequestReason = Response::Error::Reason::Success;
 };
 
 class OnlineFileSource::Impl {
@@ -169,7 +166,7 @@ OnlineFileRequestImpl::OnlineFileRequestImpl(const Resource& resource_, Callback
     if (impl.cache) {
         scheduleCacheRequest(impl);
     } else {
-        scheduleRealRequest(impl);
+        scheduleRealRequest(impl, true);
     }
 }
 
@@ -184,25 +181,28 @@ OnlineFileRequestImpl::~OnlineFileRequestImpl() {
 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.
-    cacheRequest = impl.cache->get(resource, [this, &impl](std::shared_ptr<Response> response_) {
+    cacheRequest = impl.cache->get(resource, [this, &impl](std::shared_ptr<Response> response) {
         cacheRequest = nullptr;
 
-        if (response_) {
-            response = response_;
+        if (response) {
+            resource.priorModified = response->modified;
+            resource.priorExpires = response->expires;
+            resource.priorEtag = response->etag;
             callback(*response);
         }
 
-        // Force immediate revalidation if the cached response didn't indicate an expiration. If
-        // it did indicate an expiration, revalidation will happen in the normal scheduling flow.
-        scheduleRealRequest(impl, response && !response->expires);
+        // Force immediate revalidation if we don't have a cached response, or the cached
+        // response does not have an expiration time. Otherwise revalidation will happen in
+        // the normal scheduling flow.
+        scheduleRealRequest(impl, !response || !response->expires);
     });
 }
 
-static Duration errorRetryTimeout(const Response& response, uint32_t failedRequests) {
-    if (response.error && response.error->reason == Response::Error::Reason::Server) {
+static Duration errorRetryTimeout(Response::Error::Reason failedRequestReason, uint32_t failedRequests) {
+    if (failedRequestReason == Response::Error::Reason::Server) {
         // Retry after one second three times, then start exponential backoff.
         return Seconds(failedRequests <= 3 ? 1 : 1 << std::min(failedRequests - 3, 31u));
-    } else if (response.error && response.error->reason == Response::Error::Reason::Connection) {
+    } else if (failedRequestReason == Response::Error::Reason::Connection) {
         // Immediate exponential backoff.
         assert(failedRequests > 0);
         return Seconds(1 << std::min(failedRequests - 1, 31u));
@@ -212,9 +212,9 @@ static Duration errorRetryTimeout(const Response& response, uint32_t failedReque
     }
 }
 
-static Duration expirationTimeout(const Response& response) {
-    if (response.expires) {
-        return std::max(SystemDuration::zero(), *response.expires - SystemClock::now());
+static Duration expirationTimeout(optional<SystemTimePoint> expires) {
+    if (expires) {
+        return std::max(SystemDuration::zero(), *expires - SystemClock::now());
     } else {
         return Duration::max();
     }
@@ -226,47 +226,65 @@ void OnlineFileRequestImpl::scheduleRealRequest(OnlineFileSource::Impl& impl, bo
         return;
     }
 
-    Duration timeout = Duration::zero();
-
-    // If there was a prior response and we're not being asked for a forced refresh, calculate a timeout
-    // that depends on how many consecutive errors we've encountered, and on the expiration time.
-    if (response && !forceImmediate) {
-        timeout = std::min(errorRetryTimeout(*response, failedRequests),
-                           expirationTimeout(*response));
+    // If we're not being asked for a forced refresh, calculate a timeout that depends on how many
+    // consecutive errors we've encountered, and on the expiration time, if present.
+    Duration timeout = forceImmediate
+        ? Duration::zero()
+        : std::min(errorRetryTimeout(failedRequestReason, failedRequests),
+                   expirationTimeout(resource.priorExpires));
 
-        if (timeout == Duration::max()) {
-            return;
-        }
+    if (timeout == Duration::max()) {
+        return;
     }
 
     realRequestTimer.start(timeout, Duration::zero(), [this, &impl] {
         assert(!realRequest);
-        realRequest = impl.httpContext->createRequest(resource.url, [this, &impl](std::shared_ptr<const Response> response_) {
+        realRequest = impl.httpContext->createRequest(resource, [this, &impl](Response response) {
             realRequest = nullptr;
 
-            if (impl.cache) {
-                impl.cache->put(resource, *response_);
+            // If we didn't get various caching headers in the response, continue using the
+            // previous values. Otherwise, update the previous values to the new values.
+
+            if (!response.modified) {
+                response.modified = resource.priorModified;
+            } else {
+                resource.priorModified = response.modified;
             }
 
-            response = response_;
+            if (!response.expires) {
+                response.expires = resource.priorExpires;
+            } else {
+                resource.priorExpires = response.expires;
+            }
+
+            if (!response.etag) {
+                response.etag = resource.priorEtag;
+            } else {
+                resource.priorEtag = response.etag;
+            }
 
-            if (response->error) {
+            if (impl.cache) {
+                impl.cache->put(resource, response);
+            }
+
+            if (response.error) {
                 failedRequests++;
+                failedRequestReason = response.error->reason;
             } else {
-                // Reset the number of subsequent failed requests after we got a successful one.
                 failedRequests = 0;
+                failedRequestReason = Response::Error::Reason::Success;
             }
 
-            callback(*response);
+            callback(response);
             scheduleRealRequest(impl);
-        }, response);
+        });
     });
 }
 
 void OnlineFileRequestImpl::networkIsReachableAgain(OnlineFileSource::Impl& impl) {
     // We need all requests to fail at least once before we are going to start retrying
     // them, and we only immediately restart request that failed due to connection issues.
-    if (response && response->error && response->error->reason == Response::Error::Reason::Connection) {
+    if (failedRequestReason == Response::Error::Reason::Connection) {
         scheduleRealRequest(impl, true);
     }
 }
diff --git a/src/mbgl/storage/http_context_base.hpp b/src/mbgl/storage/http_context_base.hpp
index a1a3f5b597..b50bf11ec7 100644
--- a/src/mbgl/storage/http_context_base.hpp
+++ b/src/mbgl/storage/http_context_base.hpp
@@ -13,9 +13,7 @@ public:
     static std::unique_ptr<HTTPContextBase> createContext();
 
     virtual ~HTTPContextBase() = default;
-    virtual HTTPRequestBase* createRequest(const std::string& url,
-                                       HTTPRequestBase::Callback,
-                                       std::shared_ptr<const Response>) = 0;
+    virtual HTTPRequestBase* createRequest(const Resource&, HTTPRequestBase::Callback) = 0;
 };
 
 } // namespace mbgl
diff --git a/src/mbgl/storage/http_request_base.hpp b/src/mbgl/storage/http_request_base.hpp
index 1a8cf73da6..37a0b4d091 100644
--- a/src/mbgl/storage/http_request_base.hpp
+++ b/src/mbgl/storage/http_request_base.hpp
@@ -1,14 +1,14 @@
 #ifndef MBGL_STORAGE_HTTP_REQUEST_BASE
 #define MBGL_STORAGE_HTTP_REQUEST_BASE
 
+#include <mbgl/storage/response.hpp>
+#include <mbgl/storage/resource.hpp>
+
 #include <mbgl/util/noncopyable.hpp>
 #include <mbgl/util/chrono.hpp>
 #include <mbgl/util/optional.hpp>
 
-#include <memory>
 #include <functional>
-#include <utility>
-#include <string>
 
 namespace mbgl {
 
@@ -16,10 +16,10 @@ class Response;
 
 class HTTPRequestBase : private util::noncopyable {
 public:
-    using Callback = std::function<void (std::shared_ptr<const Response> response)>;
+    using Callback = std::function<void (Response)>;
 
-    HTTPRequestBase(const std::string& url_, Callback notify_)
-        : url(url_)
+    HTTPRequestBase(const Resource& resource_, Callback notify_)
+        : resource(resource_)
         , notify(std::move(notify_))
         , cancelled(false) {
     }
@@ -30,7 +30,7 @@ public:
 protected:
     static optional<SystemTimePoint> parseCacheControl(const char *value);
 
-    std::string url;
+    Resource resource;
     Callback notify;
     bool cancelled;
 };
diff --git a/src/mbgl/storage/sqlite_cache.hpp b/src/mbgl/storage/sqlite_cache.hpp
index 64c683f4ef..b5a7cbcc07 100644
--- a/src/mbgl/storage/sqlite_cache.hpp
+++ b/src/mbgl/storage/sqlite_cache.hpp
@@ -10,7 +10,7 @@
 
 namespace mbgl {
 
-struct Resource;
+class Resource;
 class Response;
 class WorkRequest;
 
diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp
index d46f95801e..10dee80a00 100644
--- a/test/storage/cache_revalidate.cpp
+++ b/test/storage/cache_revalidate.cpp
@@ -40,8 +40,7 @@ TEST_F(Storage, CacheRevalidateSame) {
 
                 EXPECT_EQ(nullptr, res2.error);
                 EXPECT_TRUE(res2.notModified);
-                ASSERT_TRUE(res2.data.get());
-                EXPECT_EQ("Response", *res2.data);
+                ASSERT_FALSE(res2.data.get());
                 EXPECT_TRUE(bool(res2.expires));
                 EXPECT_FALSE(bool(res2.modified));
                 // We're not sending the ETag in the 304 reply, but it should still be there.
@@ -92,8 +91,7 @@ TEST_F(Storage, CacheRevalidateModified) {
 
                 EXPECT_EQ(nullptr, res2.error);
                 EXPECT_TRUE(res2.notModified);
-                ASSERT_TRUE(res2.data.get());
-                EXPECT_EQ("Response", *res2.data);
+                ASSERT_FALSE(res2.data.get());
                 EXPECT_TRUE(bool(res2.expires));
                 EXPECT_EQ(SystemClock::from_time_t(1420070400), *res2.modified);
                 EXPECT_FALSE(res2.etag);