[node] don't fire callback for canceled AsyncRequest

4 files changed, 67 insertions, 4 deletions

diff --git a/platform/node/src/node_map.cpp b/platform/node/src/node_map.cpp
index 7ed826275f..e56f31b0e6 100644
--- a/platform/node/src/node_map.cpp
+++ b/platform/node/src/node_map.cpp
@@ -528,12 +528,13 @@ std::unique_ptr<mbgl::AsyncRequest> NodeMap::request(const mbgl::Resource& resou
     Nan::HandleScope scope;
 
     auto requestHandle = NodeRequest::Create(resource, callback_)->ToObject();
+    auto request = Nan::ObjectWrap::Unwrap<NodeRequest>(requestHandle);
     auto callbackHandle = Nan::New<v8::Function>(NodeRequest::Respond, requestHandle);
 
     v8::Local<v8::Value> argv[] = { requestHandle, callbackHandle };
     Nan::MakeCallback(handle()->GetInternalField(1)->ToObject(), "request", 2, argv);
 
-    return std::make_unique<mbgl::AsyncRequest>();
+    return std::make_unique<NodeRequest::NodeAsyncRequest>(request);
 }
 
-}
+} // namespace node_mbgl
diff --git a/platform/node/src/node_request.cpp b/platform/node/src/node_request.cpp
index d1a40a9080..fa560ed4e7 100644
--- a/platform/node/src/node_request.cpp
+++ b/platform/node/src/node_request.cpp
@@ -45,6 +45,14 @@ v8::Handle<v8::Object> NodeRequest::Create(const mbgl::Resource& resource, mbgl:
 NAN_METHOD(NodeRequest::Respond) {
     using Error = mbgl::Response::Error;
 
+    // Move out of the object so callback() can only be fired once.
+    auto request = Nan::ObjectWrap::Unwrap<NodeRequest>(info.Data().As<v8::Object>());
+    auto callback = std::move(request->callback);
+    if (!callback) {
+        info.GetReturnValue().SetUndefined();
+        return;
+    }
+
     mbgl::Response response;
 
     if (info.Length() < 1) {
@@ -114,14 +122,39 @@ NAN_METHOD(NodeRequest::Respond) {
     }
 
     // Send the response object to the NodeFileSource object
-    Nan::ObjectWrap::Unwrap<NodeRequest>(info.Data().As<v8::Object>())->callback(response);
+    callback(response);
     info.GetReturnValue().SetUndefined();
 }
 
 ////////////////////////////////////////////////////////////////////////////////////////////////
 // Instance
 
+NodeRequest::NodeAsyncRequest::NodeAsyncRequest(NodeRequest* request_) : request(request_) {
+    assert(request);
+    // Make sure the JS object has a pointer to this so that it can remove its pointer in the
+    // destructor
+    request->asyncRequest = this;
+}
+
+NodeRequest::NodeAsyncRequest::~NodeAsyncRequest() {
+    if (request) {
+        // Remove the callback function because the AsyncRequest was canceled and we are no longer
+        // interested in the result.
+        request->callback = {};
+        request->asyncRequest = nullptr;
+    }
+}
+
 NodeRequest::NodeRequest(mbgl::FileSource::Callback callback_)
-    : callback(callback_) {}
+    : callback(callback_) {
+}
 
+NodeRequest::~NodeRequest() {
+    // When this object gets garbage collected, make sure that the AsyncRequest can no longer
+    // attempt to remove the callback function this object was holding (it can't be fired anymore).
+    if (asyncRequest) {
+        asyncRequest->request = nullptr;
+    }
 }
+
+} // namespace node_mbgl
diff --git a/platform/node/src/node_request.hpp b/platform/node/src/node_request.hpp
index 8e57cb30ec..2d307a3f19 100644
--- a/platform/node/src/node_request.hpp
+++ b/platform/node/src/node_request.hpp
@@ -12,6 +12,7 @@
 namespace node_mbgl {
 
 class NodeFileSource;
+class NodeRequest;
 
 class NodeRequest : public Nan::ObjectWrap {
 public:
@@ -24,9 +25,17 @@ public:
     static Nan::Persistent<v8::Function> constructor;
 
     NodeRequest(mbgl::FileSource::Callback);
+    ~NodeRequest();
+
+    struct NodeAsyncRequest : public mbgl::AsyncRequest {
+        NodeAsyncRequest(NodeRequest*);
+        ~NodeAsyncRequest() override;
+        NodeRequest* request;
+    };
 
 private:
     mbgl::FileSource::Callback callback;
+    NodeAsyncRequest* asyncRequest = nullptr;
 };
 
 }
diff --git a/platform/node/test/js/map.test.js b/platform/node/test/js/map.test.js
index 12b17126f9..e8434bc774 100644
--- a/platform/node/test/js/map.test.js
+++ b/platform/node/test/js/map.test.js
@@ -220,6 +220,26 @@ test('Map', function(t) {
             t.end();
         });
 
+        t.test('returns an error delayed', function(t) {
+            var delay = 0;
+            var map = new mbgl.Map({
+                request: function(req, callback) {
+                    delay += 100;
+                    setTimeout(function() {
+                        callback(new Error('not found'));
+                    }, delay);
+                },
+                ratio: 1
+            });
+            map.load(style);
+            map.render({ zoom: 1 }, function(err, data) {
+                map.release();
+
+                t.ok(err, 'returns error');
+                t.end();
+            });
+        });
+
         t.test('returns an error', function(t) {
             var map = new mbgl.Map(options);
             map.load(style);