[core] Use std::unique_ptr for FileSource request

5 files changed, 49 insertions, 212 deletions

diff --git a/platform/node/README.md b/platform/node/README.md
index 2d10736826..028da8dfb1 100644
--- a/platform/node/README.md
+++ b/platform/node/README.md
@@ -53,21 +53,18 @@ npm test
 
 ## Implementing a file source
 
-When creating a `Map`, you must pass an options object (with a required `request` method, optional `cancel` method and optional 'ratio' number) as the first parameter.
+When creating a `Map`, you must pass an options object (with a required `request` method and optional 'ratio' number) as the first parameter.
 
 ```js
 var map = new mbgl.Map({
     request: function(req) {
         // TODO
     },
-    cancel: function(req) {
-        // TODO
-    },
     ratio: 2.0
 });
 ```
 
-The `request()` method starts a new request to a file, while `cancel()` tells the FileSource to cancel the request (if possible). The `ratio` sets the scale at which the map will render tiles, such as `2.0` for rendering images for high pixel density displays. The `req` parameter has two properties:
+The `request()` method starts a new request to a file. The `ratio` sets the scale at which the map will render tiles, such as `2.0` for rendering images for high pixel density displays. The `req` parameter has two properties:
 
 ```json
 {
diff --git a/platform/node/src/node_file_source.cpp b/platform/node/src/node_file_source.cpp
index 8f88919638..9a1d7c055a 100644
--- a/platform/node/src/node_file_source.cpp
+++ b/platform/node/src/node_file_source.cpp
@@ -1,139 +1,44 @@
 #include "node_file_source.hpp"
 #include "node_request.hpp"
-#include "util/async_queue.hpp"
-
-#include <mbgl/storage/request.hpp>
-#include <mbgl/util/run_loop.hpp>
 
 namespace node_mbgl {
 
-struct NodeFileSource::Action {
-    const enum : bool { Add, Cancel } type;
-    mbgl::Resource const resource;
+class NodeFileSourceRequest : public mbgl::FileRequest {
+public:
+    std::unique_ptr<mbgl::WorkRequest> workRequest;
 };
 
-NodeFileSource::NodeFileSource(v8::Local<v8::Object> options_) :
-    queue(new Queue(uv_default_loop(), [this](Action &action) {
-        if (action.type == Action::Add) {
-            processAdd(action.resource);
-        } else if (action.type == Action::Cancel) {
-            processCancel(action.resource);
-        }
-    }))
-{
+NodeFileSource::NodeFileSource(v8::Local<v8::Object> options_)
+    : nodeLoop(uv_default_loop()) {
     options.Reset(options_);
 
-    // Make sure that the queue doesn't block the loop from exiting.
-    queue->unref();
+    // This has the effect of unreffing an async handle, which otherwise would keep the
+    // default loop running. You would think we could do this in the destructor instead,
+    // but in fact the destructor might not get called if there's no GC pressure which
+    // would cause the NodeMap object which owns us to get destroyed.
+    nodeLoop.stop();
 }
 
 NodeFileSource::~NodeFileSource() {
-    queue->stop();
-    queue = nullptr;
-
     options.Reset();
 }
 
-mbgl::Request* NodeFileSource::request(const mbgl::Resource& resource, Callback callback) {
-    auto req = new mbgl::Request(resource, mbgl::util::RunLoop::getLoop(), std::move(callback));
-
-    std::lock_guard<std::mutex> lock(observersMutex);
-
-    assert(observers.find(resource) == observers.end());
-    observers[resource] = req;
-
-    // This function can be called from any thread. Make sure we're executing the actual call in the
-    // file source loop by sending it over the queue. It will be processed in processAction().
-    queue->send(Action{ Action::Add, resource });
-
-    return req;
-}
-
-void NodeFileSource::cancel(mbgl::Request* req) {
-    req->cancel();
-
-    std::lock_guard<std::mutex> lock(observersMutex);
-
-    auto it = observers.find(req->resource);
-    if (it == observers.end()) {
-        return;
-    }
-
-    observers.erase(it);
-
-    // This function can be called from any thread. Make sure we're executing the actual call in the
-    // file source loop by sending it over the queue. It will be processed in processAction().
-    queue->send(Action{ Action::Cancel, req->resource });
-
-    req->destruct();
-}
-
-void NodeFileSource::processAdd(const mbgl::Resource& resource) {
-    Nan::HandleScope scope;
-
-    // Make sure the loop stays alive as long as request is pending.
-    if (pending.empty()) {
-        queue->ref();
-    }
-
-    auto requestHandle = NodeRequest::Create(this, resource)->ToObject();
-    pending.emplace(resource, requestHandle);
-
-    auto callback = Nan::GetFunction(Nan::New<v8::FunctionTemplate>(NodeRequest::Respond, requestHandle)).ToLocalChecked();
-    callback->SetName(Nan::New("respond").ToLocalChecked());
-
-    v8::Local<v8::Value> argv[] = { requestHandle, callback };
-    Nan::MakeCallback(Nan::New(options), "request", 2, argv);
-}
-
-void NodeFileSource::processCancel(const mbgl::Resource& resource) {
-    Nan::HandleScope scope;
-
-    auto it = pending.find(resource);
-    if (it == pending.end()) {
-        // The response callback was already fired. There is no point in calling the cancelation
-        // callback because the request is already completed.
-    } else {
-        v8::Local<v8::Object> requestHandle = Nan::New(it->second);
-        it->second.Reset();
-        pending.erase(it);
-
-        // Make sure the the loop can exit when there are no pending requests.
-        if (pending.empty()) {
-            queue->unref();
-        }
-
-        if (Nan::Has(Nan::New(options), Nan::New("cancel").ToLocalChecked()).FromJust()) {
-            v8::Local<v8::Value> argv[] = { requestHandle };
-            Nan::MakeCallback(Nan::New(options), "cancel", 1, argv);
-        }
-
-        // Set the request handle in the request wrapper handle to null
-        Nan::ObjectWrap::Unwrap<NodeRequest>(requestHandle)->cancel();
-    }
-}
-
-void NodeFileSource::notify(const mbgl::Resource& resource, const std::shared_ptr<const mbgl::Response>& response) {
-    // First, remove the request, since it might be destructed at any point now.
-    auto it = pending.find(resource);
-    if (it != pending.end()) {
-        it->second.Reset();
-        pending.erase(it);
+std::unique_ptr<mbgl::FileRequest> NodeFileSource::request(const mbgl::Resource& resource, Callback callback) {
+    auto req = std::make_unique<NodeFileSourceRequest>();
 
-        // Make sure the the loop can exit when there are no pending requests.
-        if (pending.empty()) {
-            queue->unref();
-        }
-    }
+    // This function can be called from any thread. Make sure we're executing the
+    // JS implementation in the node event loop.
+    req->workRequest = nodeLoop.invokeWithCallback([this] (mbgl::Resource res, Callback cb) {
+        Nan::HandleScope scope;
 
-    std::lock_guard<std::mutex> lock(observersMutex);
+        auto requestHandle = NodeRequest::Create(res, cb)->ToObject();
+        auto callbackHandle = Nan::GetFunction(Nan::New<v8::FunctionTemplate>(NodeRequest::Respond, requestHandle)).ToLocalChecked();
 
-    auto observersIt = observers.find(resource);
-    if (observersIt == observers.end()) {
-        return;
-    }
+        v8::Local<v8::Value> argv[] = { requestHandle, callbackHandle };
+        Nan::MakeCallback(Nan::New(options), "request", 2, argv);
+    }, callback, resource);
 
-    observersIt->second->notify(response);
+    return std::move(req);
 }
 
 }
diff --git a/platform/node/src/node_file_source.hpp b/platform/node/src/node_file_source.hpp
index dc1d06a3b0..4234b791ae 100644
--- a/platform/node/src/node_file_source.hpp
+++ b/platform/node/src/node_file_source.hpp
@@ -1,6 +1,7 @@
 #pragma once
 
 #include <mbgl/storage/file_source.hpp>
+#include <mbgl/util/run_loop.hpp>
 
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wunused-parameter"
@@ -9,47 +10,18 @@
 #include <nan.h>
 #pragma GCC diagnostic pop
 
-#include <memory>
-#include <mutex>
-#include <unordered_map>
-
 namespace node_mbgl {
 
-namespace util { template <typename T> class AsyncQueue; }
-
 class NodeFileSource : public mbgl::FileSource {
 public:
     NodeFileSource(v8::Local<v8::Object>);
     ~NodeFileSource();
 
-    mbgl::Request* request(const mbgl::Resource&, Callback);
-    void cancel(mbgl::Request*);
-
-    // visiblity?
-    void notify(const mbgl::Resource&, const std::shared_ptr<const mbgl::Response>&);
+    std::unique_ptr<mbgl::FileRequest> request(const mbgl::Resource&, Callback);
 
 private:
-    struct Action;
-    using Queue = util::AsyncQueue<Action>;
-
-    void processAdd(const mbgl::Resource&);
-    void processCancel(const mbgl::Resource&);
-
     Nan::Persistent<v8::Object> options;
-
-private:
-    std::unordered_map<mbgl::Resource, Nan::Persistent<v8::Object>, mbgl::Resource::Hash> pending;
-
-    // The observers list will hold pointers to all the requests waiting
-    // for a particular resource. The access must be guarded by a mutex
-    // because the list is also accessed by a thread from the mbgl::Map
-    // object and from the main thread when notifying requests of
-    // completion. Concurrent access is specially needed when
-    // canceling a request to avoid a deadlock (see #129).
-    std::unordered_map<mbgl::Resource, mbgl::Request*, mbgl::Resource::Hash> observers;
-    std::mutex observersMutex;
-
-    Queue *queue = nullptr;
+    mbgl::util::RunLoop nodeLoop;
 };
 
 }
diff --git a/platform/node/src/node_request.cpp b/platform/node/src/node_request.cpp
index 14af3ad5eb..e08e4ffd17 100644
--- a/platform/node/src/node_request.cpp
+++ b/platform/node/src/node_request.cpp
@@ -1,6 +1,5 @@
 #include "node_request.hpp"
 #include "node_file_source.hpp"
-#include <mbgl/storage/request.hpp>
 #include <mbgl/storage/response.hpp>
 
 #include <cmath>
@@ -24,27 +23,18 @@ NAN_MODULE_INIT(NodeRequest::Init) {
 }
 
 NAN_METHOD(NodeRequest::New) {
-    // Extract the pointer from the first argument
-    if (info.Length() < 2 || !info[0]->IsExternal() || !info[1]->IsExternal()) {
-        return Nan::ThrowTypeError("Cannot create Request objects explicitly");
-    }
-
-    auto source = reinterpret_cast<NodeFileSource*>(info[0].As<v8::External>()->Value());
-    auto resource = reinterpret_cast<mbgl::Resource*>(info[1].As<v8::External>()->Value());
-    auto req = new NodeRequest(source, *resource);
+    auto req = new NodeRequest(*reinterpret_cast<mbgl::FileSource::Callback*>(info[0].As<v8::External>()->Value()));
     req->Wrap(info.This());
-
     info.GetReturnValue().Set(info.This());
 }
 
-v8::Handle<v8::Object> NodeRequest::Create(NodeFileSource* source, const mbgl::Resource& resource) {
+v8::Handle<v8::Object> NodeRequest::Create(const mbgl::Resource& resource, mbgl::FileSource::Callback callback) {
     Nan::EscapableHandleScope scope;
 
     v8::Local<v8::Value> argv[] = {
-        Nan::New<v8::External>(const_cast<NodeFileSource*>(source)),
-        Nan::New<v8::External>(const_cast<mbgl::Resource*>(&resource))
+        Nan::New<v8::External>(const_cast<mbgl::FileSource::Callback*>(&callback))
     };
-    auto instance = Nan::New(constructor)->NewInstance(2, argv);
+    auto instance = Nan::New(constructor)->NewInstance(1, argv);
 
     Nan::Set(instance, Nan::New("url").ToLocalChecked(), Nan::New(resource.url).ToLocalChecked());
     Nan::Set(instance, Nan::New("kind").ToLocalChecked(), Nan::New<v8::Integer>(int(resource.kind)));
@@ -53,48 +43,35 @@ v8::Handle<v8::Object> NodeRequest::Create(NodeFileSource* source, const mbgl::R
 }
 
 NAN_METHOD(NodeRequest::Respond) {
-    auto nodeRequest = Nan::ObjectWrap::Unwrap<NodeRequest>(info.Data().As<v8::Object>());
-
-    // Request has already been responded to, or was canceled, fail silently.
-    if (!nodeRequest->resource) {
-        return info.GetReturnValue().SetUndefined();
-    }
-
-    auto source = nodeRequest->source;
-    auto resource = std::move(nodeRequest->resource);
+    using Error = mbgl::Response::Error;
+    mbgl::Response response;
 
     if (info.Length() < 1) {
-        auto response = std::make_shared<mbgl::Response>();
-        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.error = std::make_unique<Error>(Error::Reason::NotFound);
 
+    } else if (info[0]->BooleanValue()) {
         // Store the error string.
         const Nan::Utf8String message { info[0]->ToString() };
-        using Error = mbgl::Response::Error;
-        response->error = std::make_unique<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()) {
         return Nan::ThrowTypeError("Second argument must be a response object");
+
     } else {
-        auto response = std::make_shared<mbgl::Response>();
         auto res = info[1]->ToObject();
 
         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)) {
-                response->modified = modified / 1000; // JS timestamps are milliseconds
+                response.modified = modified / 1000; // JS timestamps are milliseconds
             }
         }
 
         if (Nan::Has(res, Nan::New("expires").ToLocalChecked()).FromJust()) {
             const double expires = Nan::Get(res, Nan::New("expires").ToLocalChecked()).ToLocalChecked()->ToNumber()->Value();
             if (!std::isnan(expires)) {
-                response->expires = expires / 1000; // JS timestamps are milliseconds
+                response.expires = expires / 1000; // JS timestamps are milliseconds
             }
         }
 
@@ -102,14 +79,14 @@ NAN_METHOD(NodeRequest::Respond) {
             auto etagHandle = Nan::Get(res, Nan::New("etag").ToLocalChecked()).ToLocalChecked();
             if (etagHandle->BooleanValue()) {
                 const Nan::Utf8String etag { etagHandle->ToString() };
-                response->etag = std::string { *etag, size_t(etag.length()) };
+                response.etag = std::string { *etag, size_t(etag.length()) };
             }
         }
 
         if (Nan::Has(res, Nan::New("data").ToLocalChecked()).FromJust()) {
             auto dataHandle = Nan::Get(res, Nan::New("data").ToLocalChecked()).ToLocalChecked();
             if (node::Buffer::HasInstance(dataHandle)) {
-                response->data = std::make_shared<std::string>(
+                response.data = std::make_shared<std::string>(
                     node::Buffer::Data(dataHandle),
                     node::Buffer::Length(dataHandle)
                 );
@@ -117,26 +94,17 @@ NAN_METHOD(NodeRequest::Respond) {
                 return Nan::ThrowTypeError("Response data must be a Buffer");
             }
         }
-
-        // Send the response object to the NodeFileSource object
-        source->notify(*resource, response);
     }
 
+    // Send the response object to the NodeFileSource object
+    Nan::ObjectWrap::Unwrap<NodeRequest>(info.Data().As<v8::Object>())->callback(response);
     info.GetReturnValue().SetUndefined();
 }
 
 ////////////////////////////////////////////////////////////////////////////////////////////////
 // Instance
 
-NodeRequest::NodeRequest(NodeFileSource* source_, const mbgl::Resource& resource_)
-    : source(source_),
-    resource(std::make_unique<mbgl::Resource>(resource_)) {}
-
-NodeRequest::~NodeRequest() {
-}
-
-void NodeRequest::cancel() {
-    resource.reset();
-}
+NodeRequest::NodeRequest(mbgl::FileSource::Callback callback_)
+    : callback(callback_) {}
 
 }
diff --git a/platform/node/src/node_request.hpp b/platform/node/src/node_request.hpp
index 8e01c544eb..3a06a7b62f 100644
--- a/platform/node/src/node_request.hpp
+++ b/platform/node/src/node_request.hpp
@@ -8,8 +8,7 @@
 #pragma GCC diagnostic pop
 
 #include <mbgl/storage/resource.hpp>
-
-#include <memory>
+#include <mbgl/storage/file_source.hpp>
 
 namespace node_mbgl {
 
@@ -22,17 +21,13 @@ public:
     static NAN_METHOD(New);
     static NAN_METHOD(Respond);
 
-    static v8::Handle<v8::Object> Create(NodeFileSource*, const mbgl::Resource&);
+    static v8::Handle<v8::Object> Create(const mbgl::Resource&, mbgl::FileSource::Callback);
     static Nan::Persistent<v8::Function> constructor;
 
-    NodeRequest(NodeFileSource* source, const mbgl::Resource& resource);
-    ~NodeRequest();
-
-    void cancel();
+    NodeRequest(mbgl::FileSource::Callback);
 
 private:
-    NodeFileSource* source;
-    std::unique_ptr<mbgl::Resource> resource;
+    mbgl::FileSource::Callback callback;
 };
 
 }