[core] Make DefaultFileSource react to all NetworkStatus changes

5 files changed, 92 insertions, 10 deletions

diff --git a/platform/node/src/node_request.cpp b/platform/node/src/node_request.cpp
index 211a696cd7..14af3ad5eb 100644
--- a/platform/node/src/node_request.cpp
+++ b/platform/node/src/node_request.cpp
@@ -65,16 +65,17 @@ NAN_METHOD(NodeRequest::Respond) {
 
     if (info.Length() < 1) {
         auto response = std::make_shared<mbgl::Response>();
-        response->status = mbgl::Response::NotFound;
+        using Error = mbgl::Response::Error;
+        response->error = std::make_unique<Error>(Error::Reason::NotFound);
         source->notify(*resource, response);
     } else if (info[0]->BooleanValue()) {
         auto response = std::make_shared<mbgl::Response>();
 
-        response->status = mbgl::Response::Error;
-
         // Store the error string.
         const Nan::Utf8String message { info[0]->ToString() };
-        response->message = std::string { *message, size_t(message.length()) };
+        using Error = mbgl::Response::Error;
+        response->error = std::make_unique<Error>(
+            Error::Reason::Other, std::string{ *message, size_t(message.length()) });
 
         source->notify(*resource, response);
     } else if (info.Length() < 2 || !info[1]->IsObject()) {
@@ -83,8 +84,6 @@ NAN_METHOD(NodeRequest::Respond) {
         auto response = std::make_shared<mbgl::Response>();
         auto res = info[1]->ToObject();
 
-        response->status = mbgl::Response::Successful;
-
         if (Nan::Has(res, Nan::New("modified").ToLocalChecked()).FromJust()) {
             const double modified = Nan::Get(res, Nan::New("modified").ToLocalChecked()).ToLocalChecked()->ToNumber()->Value();
             if (!std::isnan(modified)) {
diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp
index 773875c390..64c802d770 100644
--- a/src/mbgl/storage/default_file_source.cpp
+++ b/src/mbgl/storage/default_file_source.cpp
@@ -2,6 +2,7 @@
 #include <mbgl/storage/request.hpp>
 #include <mbgl/storage/asset_context_base.hpp>
 #include <mbgl/storage/http_context_base.hpp>
+#include <mbgl/storage/network_status.hpp>
 
 #include <mbgl/storage/response.hpp>
 #include <mbgl/platform/platform.hpp>
@@ -87,7 +88,29 @@ DefaultFileSource::Impl::Impl(FileCache* cache_, const std::string& root)
       cache(cache_),
       assetRoot(root.empty() ? platform::assetRoot() : root),
       assetContext(AssetContextBase::createContext(loop)),
-      httpContext(HTTPContextBase::createContext(loop)) {
+      httpContext(HTTPContextBase::createContext(loop)),
+      reachability(std::make_unique<uv::async>(loop, std::bind(&Impl::networkIsReachableAgain, this))) {
+    // Subscribe to network status changes, but make sure that this async handle doesn't keep the
+    // loop alive; otherwise our app wouldn't terminate. After all, we only need status change
+    // notifications when our app is still running.
+    NetworkStatus::Subscribe(reachability->get());
+    reachability->unref();
+}
+
+DefaultFileSource::Impl::~Impl() {
+    NetworkStatus::Unsubscribe(reachability->get());
+}
+
+void DefaultFileSource::Impl::networkIsReachableAgain() {
+    for (auto& req : pending) {
+        auto& request = req.second;
+        auto& response = request.getResponse();
+        if (!request.realRequest && response && response->error && response->error->reason == Response::Error::Reason::Connection) {
+            // 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.
+            startRealRequest(request);
+        }
+    }
 }
 
 void DefaultFileSource::Impl::add(Request* req) {
@@ -157,6 +180,8 @@ void DefaultFileSource::Impl::startCacheRequest(DefaultFileRequest& request) {
 }
 
 void DefaultFileSource::Impl::startRealRequest(DefaultFileRequest& request) {
+    assert(!request.realRequest);
+
     // Cancel the timer if we have one.
     if (request.timerRequest) {
         request.timerRequest->stop();
diff --git a/src/mbgl/storage/default_file_source_impl.hpp b/src/mbgl/storage/default_file_source_impl.hpp
index 2e5ca93ccc..4ca8e42721 100644
--- a/src/mbgl/storage/default_file_source_impl.hpp
+++ b/src/mbgl/storage/default_file_source_impl.hpp
@@ -69,6 +69,9 @@ private:
 class DefaultFileSource::Impl {
 public:
     Impl(FileCache*, const std::string& = "");
+    ~Impl();
+
+    void networkIsReachableAgain();
 
     void add(Request*);
     void cancel(Request*);
@@ -85,6 +88,7 @@ private:
     const std::string assetRoot;
     const std::unique_ptr<AssetContextBase> assetContext;
     const std::unique_ptr<HTTPContextBase> httpContext;
+    const std::unique_ptr<uv::async> reachability;
 };
 
 }
diff --git a/test/storage/http_error.cpp b/test/storage/http_error.cpp
index 00deeb063a..94a3f29a4e 100644
--- a/test/storage/http_error.cpp
+++ b/test/storage/http_error.cpp
@@ -92,7 +92,4 @@ TEST_F(Storage, HTTPError) {
     });
 
     uv_run(uv_default_loop(), UV_RUN_DEFAULT);
-
-    // Run again so that the timer handle can be properly closed.
-    uv_run(uv_default_loop(), UV_RUN_DEFAULT);
 }
diff --git a/test/storage/http_retry_network_status.cpp b/test/storage/http_retry_network_status.cpp
index d0209e0bf6..3f8c469cf9 100644
--- a/test/storage/http_retry_network_status.cpp
+++ b/test/storage/http_retry_network_status.cpp
@@ -47,3 +47,60 @@ TEST_F(Storage, HTTPNetworkStatusChange) {
 
     uv_run(uv_default_loop(), UV_RUN_DEFAULT);
 }
+
+// Tests that a change in network status preempts requests that failed due to connection or
+// reachability issues.
+TEST_F(Storage, HTTPNetworkStatusChangePreempt) {
+    SCOPED_TEST(HTTPNetworkStatusChangePreempt)
+
+    using namespace mbgl;
+
+    DefaultFileSource fs(nullptr);
+
+    const auto start = uv_hrtime();
+
+    const Resource resource{ Resource::Unknown, "http://127.0.0.1:3001/test" };
+    Request* req = fs.request(resource, uv_default_loop(), [&](const Response& res) {
+        static int counter = 0;
+        const auto duration = double(uv_hrtime() - start) / 1e9;
+        if (counter == 0) {
+            EXPECT_GT(0.2, duration) << "Response came in too late";
+        } else if (counter == 1) {
+            EXPECT_LT(0.39, duration) << "Preempted retry triggered too early";
+            EXPECT_GT(0.6, duration) << "Preempted retry triggered too late";
+        } else if (counter > 1) {
+            FAIL() << "Retried too often";
+        }
+        ASSERT_NE(nullptr, res.error);
+        EXPECT_EQ(Response::Error::Reason::Connection, res.error->reason);
+#ifdef MBGL_HTTP_NSURL
+        EXPECT_TRUE(res.error->message ==
+                     "The operation couldn’t be completed. (NSURLErrorDomain error -1004.)" ||
+                 res.error->message == "Could not connect to the server.")
+         << "Full message is: \"" << res.error->message << "\"";
+#elif MBGL_HTTP_CURL
+        const std::string prefix { "Couldn't connect to server: " };
+        EXPECT_STREQ(prefix.c_str(), res.error->message.substr(0, prefix.size()).c_str()) << "Full message is: \"" << res.error->message << "\"";
+#else
+        FAIL();
+#endif
+        EXPECT_EQ(false, res.stale);
+        ASSERT_FALSE(res.data.get());
+        EXPECT_EQ(0, res.expires);
+        EXPECT_EQ(0, res.modified);
+        EXPECT_EQ("", res.etag);
+
+        if (counter++ == 1) {
+            fs.cancel(req);
+            HTTPNetworkStatusChangePreempt.finish();
+        }
+    });
+
+    // After 400 milliseconds, we're going to trigger a NetworkStatus change.
+    uv::timer reachableTimer(uv_default_loop());
+    reachableTimer.start(400, 0, [] () {
+        mbgl::NetworkStatus::Reachable();
+    });
+
+    uv_run(uv_default_loop(), UV_RUN_DEFAULT);
+}