[node] minor stylistic changes in NodeFileSource

4 files changed, 36 insertions, 28 deletions

diff --git a/platform/node/src/node_file_source.cpp b/platform/node/src/node_file_source.cpp
index 965e49bc66..1b05b142de 100644
--- a/platform/node/src/node_file_source.cpp
+++ b/platform/node/src/node_file_source.cpp
@@ -8,25 +8,25 @@ namespace mbgl {
 
 struct NodeFileSource::Action {
     const enum : bool { Add, Cancel } type;
-    Resource const resource;
+    const Resource resource;
 };
 
-NodeFileSource::NodeFileSource(v8::Local<v8::Object> options_) :
+NodeFileSource::NodeFileSource(v8::Local<v8::Object> options_)
+    : options(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);
         }
-    }))
-{
-    options.Reset(options_);
-
+    })) {
     // Make sure that the queue doesn't block the loop from exiting.
     queue->unref();
 }
 
 NodeFileSource::~NodeFileSource() {
+    MBGL_VERIFY_THREAD(tid);
+
     queue->stop();
     queue = nullptr;
 
@@ -34,36 +34,45 @@ NodeFileSource::~NodeFileSource() {
 }
 
 Request* NodeFileSource::request(const Resource& resource, uv_loop_t* loop, Callback callback) {
+    assert(l);
+
     auto req = new Request(resource, loop, std::move(callback));
 
     std::lock_guard<std::mutex> lock(observersMutex);
 
+    // Add this request as an observer so that it'll get notified when something about this
+    // request changes.
     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().
+    // file source loop by sending it over the queue.
     queue->send(Action{ Action::Add, resource });
 
     return req;
 }
 
 void NodeFileSource::cancel(Request* req) {
+    assert(req);
+
     req->cancel();
 
     std::lock_guard<std::mutex> lock(observersMutex);
 
     auto it = observers.find(req->resource);
     if (it == observers.end()) {
+        // Exit early if no observers are found.
         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().
+    // file source loop by sending it over the queue.
     queue->send(Action{ Action::Cancel, req->resource });
 
+    // Send a message back to the requesting thread and notify it that this request has been
+    // canceled and is now safe to be deleted.
     req->destruct();
 }
 
@@ -89,11 +98,8 @@ void NodeFileSource::processCancel(const 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);
+    if (it != pending.end()) {
+        auto requestHandle = Nan::New(it->second);
         it->second.Reset();
         pending.erase(it);
 
@@ -107,8 +113,11 @@ void NodeFileSource::processCancel(const Resource& resource) {
             Nan::MakeCallback(Nan::New(options), "cancel", 1, argv);
         }
 
-        // Set the request handle in the request wrapper handle to null
+        // Set the resource handle in the request wrapper handle to null
         Nan::ObjectWrap::Unwrap<NodeRequest>(requestHandle)->cancel();
+    } else {
+        // There is no request for this URL anymore. Likely, the request already completed
+        // before we got around to process the cancelation request.
     }
 }
 
@@ -129,6 +138,7 @@ void NodeFileSource::notify(const Resource& resource, const std::shared_ptr<cons
 
     auto observersIt = observers.find(resource);
     if (observersIt == observers.end()) {
+        // Exit early if no observers are found.
         return;
     }
 
diff --git a/platform/node/src/node_file_source.hpp b/platform/node/src/node_file_source.hpp
index 5929b96d02..514f349f92 100644
--- a/platform/node/src/node_file_source.hpp
+++ b/platform/node/src/node_file_source.hpp
@@ -9,9 +9,8 @@
 #include <nan.h>
 #pragma GCC diagnostic pop
 
-#include <memory>
-#include <mutex>
 #include <unordered_map>
+#include <mutex>
 
 namespace mbgl {
 
@@ -20,24 +19,25 @@ namespace util { template <typename T> class AsyncQueue; }
 class NodeFileSource : public FileSource {
 public:
     NodeFileSource(v8::Local<v8::Object>);
-    ~NodeFileSource();
+    ~NodeFileSource() override;
 
-    Request* request(const Resource&, uv_loop_t*, Callback);
-    void cancel(Request*);
+    // FileSource API
+    Request* request(const Resource&, uv_loop_t*, Callback) override;
+    void cancel(Request*) override;
 
-    // visiblity?
     void notify(const Resource&, const std::shared_ptr<const Response>&);
 
 private:
+    Nan::Persistent<v8::Object> options;
+
     struct Action;
     using Queue = util::AsyncQueue<Action>;
 
+    Queue *queue = nullptr;
+
     void processAdd(const Resource&);
     void processCancel(const Resource&);
 
-    Nan::Persistent<v8::Object> options;
-
-private:
     std::unordered_map<Resource, Nan::Persistent<v8::Object>, Resource::Hash> pending;
 
     // The observers list will hold pointers to all the requests waiting
@@ -45,11 +45,10 @@ private:
     // because the list is also accessed by a thread from the 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).
+    // canceling a request to avoid a deadlock 
+    // (see https://github.com/mapbox/node-mapbox-gl-native/issues/129).
     std::unordered_map<Resource, Request*, Resource::Hash> observers;
     std::mutex observersMutex;
-
-    Queue *queue = nullptr;
 };
 
 }
diff --git a/platform/node/src/node_map.hpp b/platform/node/src/node_map.hpp
index 0c4ef1d80b..07fd877c6f 100644
--- a/platform/node/src/node_map.hpp
+++ b/platform/node/src/node_map.hpp
@@ -29,7 +29,7 @@ public:
     static NAN_METHOD(Release);
     static NAN_METHOD(DumpDebugLogs);
 
-    void startRender(std::unique_ptr<NodeMap::RenderOptions> options);
+    void startRender(std::unique_ptr<NodeMap::RenderOptions>);
     void renderFinished();
 
     void release();
diff --git a/platform/node/src/node_request.cpp b/platform/node/src/node_request.cpp
index ee00d46216..d109a0f3e1 100644
--- a/platform/node/src/node_request.cpp
+++ b/platform/node/src/node_request.cpp
@@ -4,7 +4,6 @@
 #include <mbgl/storage/response.hpp>
 
 #include <cmath>
-#include <iostream>
 
 namespace mbgl {