[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.

4 files changed, 104 insertions, 159 deletions

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