From 8582120194ff7e6fe42cf2fe515712f5a979bf9e Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Thu, 12 Nov 2015 16:19:08 -0500 Subject: [node] minor stylistic changes in NodeFileSource --- platform/node/src/node_file_source.cpp | 38 +++++++++++++++++++++------------- platform/node/src/node_file_source.hpp | 23 ++++++++++---------- platform/node/src/node_map.hpp | 2 +- platform/node/src/node_request.cpp | 1 - 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 options_) : +NodeFileSource::NodeFileSource(v8::Local 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 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 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 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(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 #pragma GCC diagnostic pop -#include -#include #include +#include namespace mbgl { @@ -20,24 +19,25 @@ namespace util { template class AsyncQueue; } class NodeFileSource : public FileSource { public: NodeFileSource(v8::Local); - ~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&); private: + Nan::Persistent options; + struct Action; using Queue = util::AsyncQueue; + Queue *queue = nullptr; + void processAdd(const Resource&); void processCancel(const Resource&); - Nan::Persistent options; - -private: std::unordered_map, 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 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 options); + void startRender(std::unique_ptr); 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 #include -#include namespace mbgl { -- cgit v1.2.1