From d515f8d77db11146d1548642e6bc17b8e3c0ab3d Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Thu, 4 Jun 2015 16:20:31 +0300 Subject: [PATCH 1/6] Bump Mapbox GL Native to v0.3.3 --- vendor/mbgl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vendor/mbgl b/vendor/mbgl index a96a251de..14083700e 160000 --- a/vendor/mbgl +++ b/vendor/mbgl @@ -1 +1 @@ -Subproject commit a96a251de9cad66c60fbc3f3cb8376c5d53df25e +Subproject commit 14083700e5e85119adf134e6edbcbfd54cfd33b7 From 34ef7ac003b1f44fd1a1a1db052b0905b9a4a77c Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Thu, 11 Jun 2015 17:42:48 +0300 Subject: [PATCH 2/6] Implement request coalescing and synchronous request cancellation When requests are made to the FileSource, we now group them by resource, similarly to what is done on the native implementation. The file source will reuse the same Response for all the requests on the observer list. The NodeRequest no longer holds a reference to the mbgl::Request, so we can cancel it synchronously. This is needed to avoid a deadlock when releasing the NodeMap resources with pending requests. Before: - Main thread | NodeMap::release() - Main thread | mbgl::~Map() - Main thread | Blocks waiting for the Map thread to join - Map thread | mbgl::Request::cancel() - Map thread | NodeFileSource posts the "cancel" message to main thread - *deadlock* | Map thread cannot join because mbgl::Request holds a reference to the Map thread main loop. Main thread is blocked, never gets the "cancel" message. Now: - Main thread | NodeMap::release() - Main thread | mbgl::~Map() - Main thread | Blocks waiting for the Map thread to join - Map thread | Request::cancel() - Map thread | NodeFileSource::cancel(), destroys the mbgl::Request synchronously, mutex guarded access. We also send a notification to cancel the NodeRequest on the main thread. - Map thread | Map thread joins. - Main thread | NodeMap::release() returns - Main thread | NodeRequest gets canceled. If NodeRequest replies before it gets the "cancel" message, nothing happens, notification gets discarded. --- src/node_file_source.cpp | 84 ++++++++++++++++++++++++++++++---------- src/node_file_source.hpp | 25 +++++++++--- src/node_request.cpp | 29 +++++++------- src/node_request.hpp | 11 ++++-- 4 files changed, 104 insertions(+), 45 deletions(-) diff --git a/src/node_file_source.cpp b/src/node_file_source.cpp index 6f4a05bca..935bac427 100644 --- a/src/node_file_source.cpp +++ b/src/node_file_source.cpp @@ -4,6 +4,8 @@ #include +#include + namespace node_mbgl { //////////////////////////////////////////////////////////////////////////////////////////////// @@ -42,15 +44,15 @@ NAN_METHOD(NodeFileSource::New) { struct NodeFileSource::Action { const enum : bool { Add, Cancel } type; - mbgl::Request *const request; + mbgl::Resource const resource; }; NodeFileSource::NodeFileSource() : queue(new Queue(uv_default_loop(), [this](Action &action) { if (action.type == Action::Add) { - processAdd(action.request); + processAdd(action.resource); } else if (action.type == Action::Cancel) { - processCancel(action.request); + processCancel(action.resource); } })) { @@ -66,21 +68,50 @@ NodeFileSource::~NodeFileSource() { mbgl::Request* NodeFileSource::request(const mbgl::Resource& resource, uv_loop_t* loop, Callback callback) { auto req = new mbgl::Request(resource, loop, std::move(callback)); - // 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, req }); + std::lock_guard lock(observersMutex); + auto it = observers.find(resource); + if (it == 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 }); + } else { + it->second.emplace_back(req); + } + return req; } void NodeFileSource::cancel(mbgl::Request* req) { req->cancel(); - // 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 }); + std::lock_guard lock(observersMutex); + + auto observersIter = observers.find(req->resource); + if (observersIter == observers.end()) { + return; + } + + auto& observersList = observersIter->second; + auto observersListIter = std::find(observersList.begin(), observersList.end(), req); + if (observersListIter == observersList.end()) { + return; + } + + observersList.erase(observersListIter); + if (observersList.empty()) { + observers.erase(observersIter); + + // 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(mbgl::Request *req) { +void NodeFileSource::processAdd(const mbgl::Resource& resource) { NanScope(); // Make sure the loop stays alive as long as request is pending. @@ -89,23 +120,23 @@ void NodeFileSource::processAdd(mbgl::Request *req) { } auto handle = NanObjectWrapHandle(this); - auto requestHandle = NanNew(NodeRequest::Create(handle, req)); + auto requestHandle = NanNew(NodeRequest::Create(handle, resource)); #if (NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION) const v8::UniquePersistent requestPersistent(v8::Isolate::GetCurrent(), requestHandle); #else v8::Persistent requestPersistent; NanAssignPersistent(requestPersistent, requestHandle); #endif - pending.emplace(req, std::move(requestPersistent)); + pending.emplace(resource, std::move(requestPersistent)); v8::Local argv[] = { requestHandle }; NanMakeCallback(handle, NanNew("request"), 1, argv); } -void NodeFileSource::processCancel(mbgl::Request *req) { +void NodeFileSource::processCancel(const mbgl::Resource& resource) { NanScope(); - auto it = pending.find(req); + 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. @@ -136,14 +167,11 @@ void NodeFileSource::processCancel(mbgl::Request *req) { // Set the request handle in the request wrapper handle to null ObjectWrap::Unwrap(requestHandle)->cancel(); } - - // Finally, destruct the request object - req->destruct(); } -void NodeFileSource::notify(mbgl::Request *req, const std::shared_ptr& response) { +void NodeFileSource::notify(const mbgl::Resource& resource, const std::shared_ptr& response) { // First, remove the request, since it might be destructed at any point now. - auto it = pending.find(req); + auto it = pending.find(resource); if (it != pending.end()) { #if (NODE_MODULE_VERSION <= NODE_0_10_MODULE_VERSION) NanDisposePersistent(it->second); @@ -156,7 +184,23 @@ void NodeFileSource::notify(mbgl::Request *req, const std::shared_ptrnotify(response); + std::vector observersList; + + { + std::lock_guard lock(observersMutex); + + auto observersIter = observers.find(resource); + if (observersIter == observers.end()) { + return; + } + + observersList.swap(observersIter->second); + observers.erase(observersIter); + } + + for (auto request : observersList) { + request->notify(response); + } } } diff --git a/src/node_file_source.hpp b/src/node_file_source.hpp index e69a8fcf1..5220cd235 100644 --- a/src/node_file_source.hpp +++ b/src/node_file_source.hpp @@ -10,8 +10,10 @@ #include #pragma GCC diagnostic pop -#include #include +#include +#include +#include namespace node_mbgl { @@ -36,21 +38,32 @@ public: void cancel(mbgl::Request*); // visiblity? - void notify(mbgl::Request*, const std::shared_ptr&); + void notify(const mbgl::Resource&, const std::shared_ptr&); private: struct Action; using Queue = util::AsyncQueue; - void processAdd(mbgl::Request*); - void processCancel(mbgl::Request*); + void processAdd(const mbgl::Resource&); + void processCancel(const mbgl::Resource&); private: #if (NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION) - std::map &> pending; + std::unordered_map &, mbgl::Resource::Hash> pending; #else - std::map> pending; + std::unordered_map, mbgl::Resource::Hash> pending; #endif + + // The observers list will hold pointers to all the requests waiting + // for a particular resource. It is also used for coalescing requests, + // so we don't ask for the same resources twice. 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::Hash> observers; + std::mutex observersMutex; + Queue *queue = nullptr; }; diff --git a/src/node_request.cpp b/src/node_request.cpp index ccac813d2..e01162ff5 100644 --- a/src/node_request.cpp +++ b/src/node_request.cpp @@ -35,35 +35,34 @@ NAN_METHOD(NodeRequest::New) { return NanThrowTypeError("Cannot create Request objects explicitly"); } - auto request = reinterpret_cast(args[1].As()->Value()); - auto req = new NodeRequest(args[0]->ToObject(), request); + auto resource = reinterpret_cast(args[1].As()->Value()); + auto req = new NodeRequest(args[0]->ToObject(), *resource); req->Wrap(args.This()); NanReturnValue(args.This()); } -v8::Handle NodeRequest::Create(v8::Handle source, mbgl::Request *request) { +v8::Handle NodeRequest::Create(v8::Handle source, const mbgl::Resource& resource) { NanEscapableScope(); - v8::Local argv[] = { NanNew(source), NanNew(request) }; + v8::Local argv[] = { NanNew(source), + NanNew(const_cast(&resource)) }; auto instance = NanNew(constructorTemplate)->GetFunction()->NewInstance(2, argv); - instance->ForceSet(NanNew("url"), NanNew(request->resource.url), v8::ReadOnly); - instance->ForceSet(NanNew("kind"), NanNew(int(request->resource.kind)), v8::ReadOnly); + instance->ForceSet(NanNew("url"), NanNew(resource.url), v8::ReadOnly); + instance->ForceSet(NanNew("kind"), NanNew(int(resource.kind)), v8::ReadOnly); return NanEscapeScope(instance); } NAN_METHOD(NodeRequest::Respond) { auto nodeRequest = ObjectWrap::Unwrap(args.Holder()); - if (!nodeRequest->request) { + if (!nodeRequest->resource) { return NanThrowError("Request has already been responded to, or was canceled."); } auto source = ObjectWrap::Unwrap(NanNew(nodeRequest->source)); - - auto request = nodeRequest->request; - nodeRequest->request = nullptr; + auto resource = std::move(nodeRequest->resource); if (args.Length() < 1) { return NanThrowTypeError("First argument must be an error object"); @@ -76,7 +75,7 @@ NAN_METHOD(NodeRequest::Respond) { const NanUtf8String message { args[0]->ToString() }; response->message = std::string { *message, size_t(message.length()) }; - source->notify(request, response); + source->notify(*resource, response); } else if (args.Length() < 2 || !args[1]->IsObject()) { return NanThrowTypeError("Second argument must be a response object"); } else { @@ -120,7 +119,7 @@ NAN_METHOD(NodeRequest::Respond) { } // Send the response object to the NodeFileSource object - source->notify(request, response); + source->notify(*resource, response); } NanReturnUndefined(); @@ -129,8 +128,8 @@ NAN_METHOD(NodeRequest::Respond) { //////////////////////////////////////////////////////////////////////////////////////////////// // Instance -NodeRequest::NodeRequest(v8::Local source_, mbgl::Request *request_) - : request(request_) { +NodeRequest::NodeRequest(v8::Local source_, const mbgl::Resource& resource_) + : resource(std::make_unique(resource_)) { NanAssignPersistent(source, source_); } @@ -139,7 +138,7 @@ NodeRequest::~NodeRequest() { } void NodeRequest::cancel() { - request = nullptr; + resource.reset(); } } diff --git a/src/node_request.hpp b/src/node_request.hpp index 009855066..0fbf91fc6 100644 --- a/src/node_request.hpp +++ b/src/node_request.hpp @@ -7,8 +7,11 @@ #include #pragma GCC diagnostic pop +#include + +#include + class NodeFileSource; -namespace mbgl { class Request; } namespace node_mbgl { @@ -20,21 +23,21 @@ public: static NAN_METHOD(New); static NAN_METHOD(Respond); - static v8::Handle Create(v8::Handle source, mbgl::Request *request); + static v8::Handle Create(v8::Handle source, const mbgl::Resource& resource); static v8::Persistent constructorTemplate; //////////////////////////////////////////////////////////////////////////////////////////////// // Instance public: - NodeRequest(v8::Local source, mbgl::Request *request); + NodeRequest(v8::Local source, const mbgl::Resource& resource); ~NodeRequest(); void cancel(); private: v8::Persistent source; - mbgl::Request *request = nullptr; + std::unique_ptr resource; }; } From b602835626dfc1532bf5c26b9d2c34054a2d8fac Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Mon, 15 Jun 2015 01:55:20 +0300 Subject: [PATCH 3/6] Fix invalid NodeRequest reference On the code path built with node v0.12.4, we are storing an invalid reference to NodeRequest on the "pending" list. It will crash when we need to access it like we do when canceling a request. Ideally we should just have a "std::unordered_map" but that is not possible because the move semantics are not fully implemented until V8 v4.3.22. It can be used on a std::vector but not on a std::map. See: https://code.google.com/p/v8/issues/detail?id=3669 The solution for keep using C++11 move semantics is to declare the v8::CopyablePersistentTraits shenanigans and explicitly clear the v8::Persistent (like we do already for node v0.10). When Node gets V8 v4.3.22+ we can go back to v8::UniquePersistent with a more clear implementation. --- src/node_file_source.cpp | 14 +++++--------- src/node_file_source.hpp | 2 +- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/node_file_source.cpp b/src/node_file_source.cpp index 935bac427..3409f03af 100644 --- a/src/node_file_source.cpp +++ b/src/node_file_source.cpp @@ -121,12 +121,9 @@ void NodeFileSource::processAdd(const mbgl::Resource& resource) { auto handle = NanObjectWrapHandle(this); auto requestHandle = NanNew(NodeRequest::Create(handle, resource)); -#if (NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION) - const v8::UniquePersistent requestPersistent(v8::Isolate::GetCurrent(), requestHandle); -#else + v8::Persistent requestPersistent; NanAssignPersistent(requestPersistent, requestHandle); -#endif pending.emplace(resource, std::move(requestPersistent)); v8::Local argv[] = { requestHandle }; @@ -143,12 +140,9 @@ void NodeFileSource::processCancel(const mbgl::Resource& resource) { } else { #if (NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION) auto requestHandle = v8::Local::New(v8::Isolate::GetCurrent(), it->second); + it->second.Reset(); #else auto requestHandle = NanNew(it->second); -#endif - - // Dispose and remove the persistent handle -#if (NODE_MODULE_VERSION <= NODE_0_10_MODULE_VERSION) NanDisposePersistent(it->second); #endif pending.erase(it); @@ -173,7 +167,9 @@ void NodeFileSource::notify(const mbgl::Resource& resource, const std::shared_pt // First, remove the request, since it might be destructed at any point now. auto it = pending.find(resource); if (it != pending.end()) { -#if (NODE_MODULE_VERSION <= NODE_0_10_MODULE_VERSION) +#if (NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION) + it->second.Reset(); +#else NanDisposePersistent(it->second); #endif pending.erase(it); diff --git a/src/node_file_source.hpp b/src/node_file_source.hpp index 5220cd235..06b1c3d32 100644 --- a/src/node_file_source.hpp +++ b/src/node_file_source.hpp @@ -49,7 +49,7 @@ private: private: #if (NODE_MODULE_VERSION > NODE_0_10_MODULE_VERSION) - std::unordered_map &, mbgl::Resource::Hash> pending; + std::unordered_map>, mbgl::Resource::Hash> pending; #else std::unordered_map, mbgl::Resource::Hash> pending; #endif From 0d2543675fe1b1ce94147629fff2e90137f1d49f Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Wed, 3 Jun 2015 16:33:50 +0300 Subject: [PATCH 4/6] Update the tests Reflect the changes on the previous MBGL version bump and unskip tests that should pass now. --- test/js/gzip.test.js | 2 +- test/js/map.test.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/js/gzip.test.js b/test/js/gzip.test.js index ed1bb3c92..2ed0080ac 100644 --- a/test/js/gzip.test.js +++ b/test/js/gzip.test.js @@ -104,7 +104,7 @@ test('gzip', function(t) { mbgl.once('message', function(msg) { if (msg.severity == 'ERROR') { t.ok(msg, 'emits error'); - t.equal(msg.class, 'ResourceLoader'); + t.equal(msg.class, 'Style'); t.equal(msg.severity, 'ERROR'); t.ok(msg.text.match(/pbf unknown field type exception/), 'error text matches'); } diff --git a/test/js/map.test.js b/test/js/map.test.js index a29f661d8..ce6017beb 100644 --- a/test/js/map.test.js +++ b/test/js/map.test.js @@ -220,10 +220,10 @@ test('Map', function(t) { }); }); - t.skip('returns an error', function(t) { + t.test('returns an error', function(t) { mbgl.on('message', function(msg) { t.ok(msg, 'emits error'); - t.equal(msg.class, 'ResourceLoader'); + t.equal(msg.class, 'Style'); t.equal(msg.severity, 'ERROR'); t.ok(msg.text.match(/Failed to load/), 'error text matches'); }); From a6ea979ec5dc4383b90fcf7d7c69c1718a7baf84 Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Fri, 12 Jun 2015 17:08:57 +0300 Subject: [PATCH 5/6] Add test case for the request coalescing --- test/js/map.test.js | 138 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/test/js/map.test.js b/test/js/map.test.js index ce6017beb..57283c4dd 100644 --- a/test/js/map.test.js +++ b/test/js/map.test.js @@ -167,6 +167,144 @@ test('Map', function(t) { t.end(); }); + t.test('request coalesting', function(t) { + var requestList = []; + var fileSourceDoNotReply = new mbgl.FileSource(); + + // This file source will never reply to any + // request other than "./fixtures/tiles.tilejson" which + // will force the cancellation of the pending requests. + fileSourceDoNotReply.request = function(req) { + if (req.url != "./fixtures/tiles.tilejson") { + requestList.push(req); + return; + } + + fs.readFile(path.join('test', req.url), function(err, data) { + req.respond(err, { data: data }); + }); + }; + + fileSourceDoNotReply.cancel = function() { + requestList.pop(); + }; + + t.test('merge requests/cancels for the same resource', function(t) { + var map1 = new mbgl.Map(fileSourceDoNotReply); + var map2 = new mbgl.Map(fileSourceDoNotReply); + var map3 = new mbgl.Map(fileSourceDoNotReply); + var map4 = new mbgl.Map(fileSourceDoNotReply); + var map5 = new mbgl.Map(fileSourceDoNotReply); + + map1.load(style); + map2.load(style); + map3.load(style); + map4.load(style); + map5.load(style); + + var renderCallback = function(err, data) {} + + map1.render({}, renderCallback); + map2.render({}, renderCallback); + map3.render({}, renderCallback); + map4.render({}, renderCallback); + map5.render({}, renderCallback); + + var checkCancel = function() { + t.equal(requestList.length, 0); + t.end(); + }; + + var checkCoalescing = function() { + // Check if all the 5 vector tile + // requests are going to merge in + // a single request. + t.equal(requestList.length, 1); + + // This should cause the pending + // resources to get canceled, but + // only one cancel is emitted, because + // all map objects requested the same + // resource. + map1.release(); + map2.release(); + map3.release(); + map4.release(); + map5.release(); + + setTimeout(checkCancel, 500); + }; + + setTimeout(checkCoalescing, 500); + }); + + var vectorTileRequestCount = 0; + var fileSourceDelay = new mbgl.FileSource(); + + // This file source will add a little delay before + // replying so we make sure that all the map objects + // request the same resource will get a reply from the + // same request. + fileSourceDelay.request = function(req) { + var timeout = 0; + + if (req.url != "./fixtures/tiles.tilejson") { + timeout = 500; + t.ok(++vectorTileRequestCount == 1, "Should make only one request for tiles"); + } + + var readFile = function() { + fs.readFile(path.join('test', req.url), function(err, data) { + req.respond(err, { data: data }); + }); + }; + + setTimeout(readFile, timeout); + }; + + fileSourceDelay.cancel = function() { + t.fail("Should never cancel"); + }; + + t.test('one request for the same resource notifies multiple map objects', function(t) { + var map1 = new mbgl.Map(fileSourceDelay); + var map2 = new mbgl.Map(fileSourceDelay); + var map3 = new mbgl.Map(fileSourceDelay); + var map4 = new mbgl.Map(fileSourceDelay); + var map5 = new mbgl.Map(fileSourceDelay); + + map1.load(style); + map2.load(style); + map3.load(style); + map4.load(style); + map5.load(style); + + var renderFinishedCount = 0; + var renderCallback = function(err, data) { + // We make only one request for tiles, but that + // should be enough for all these map objects because + // they all want the same resource. + if (++renderFinishedCount == 5) { + map1.release(); + map2.release(); + map3.release(); + map4.release(); + map5.release(); + + t.end(); + } + } + + map1.render({}, renderCallback); + map2.render({}, renderCallback); + map3.render({}, renderCallback); + map4.render({}, renderCallback); + map5.render({}, renderCallback); + }); + + t.end(); + }); + t.test('render argument requirements', function(t) { var fileSource = new mbgl.FileSource(); fileSource.request = function(req) { From e06bf232f44e09db4c628375441586cb994cc6a1 Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Mon, 15 Jun 2015 14:52:35 -0400 Subject: [PATCH 6/6] request coalescing fixes #114 --- test/js/map.test.js | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/test/js/map.test.js b/test/js/map.test.js index 57283c4dd..427cf586b 100644 --- a/test/js/map.test.js +++ b/test/js/map.test.js @@ -132,11 +132,7 @@ test('Map', function(t) { }); }); - t.skip('accepts a JSON stylesheet', { timeout: 1000 }, function(t) { - mbgl.once('message', function(msg) { - console.log(msg); - }); - + t.test('accepts a JSON stylesheet', { timeout: 1000 }, function(t) { setup(fileSource, function(map) { t.doesNotThrow(function() { map.load(style); @@ -148,11 +144,7 @@ test('Map', function(t) { }); }); - t.skip('accepts a stringified stylesheet', { timeout: 1000 }, function(t) { - mbgl.once('message', function(msg) { - console.log(msg); - }); - + t.test('accepts a stringified stylesheet', { timeout: 1000 }, function(t) { setup(fileSource, function(map) { t.doesNotThrow(function() { map.load(JSON.stringify(style));