Fixes problems associated with node 10 and NAN#14847
Conversation
| @@ -1244,20 +1246,19 @@ std::unique_ptr<mbgl::AsyncRequest> NodeFileSource::request(const mbgl::Resource | |||
| // *this while we're still executing code. | |||
| nodeMap->handle(); | |||
There was a problem hiding this comment.
I know it is not part of this PR, but how is this holding the reference if we don't assign it to anything?
There was a problem hiding this comment.
I have no idea what this line does at all.
| struct NodeAsyncRequest : public mbgl::AsyncRequest { | ||
| NodeAsyncRequest(); | ||
| ~NodeAsyncRequest() override; | ||
| NodeRequest* request; |
There was a problem hiding this comment.
This is already set to nullptr here - do you want this changed? https://github.com/mapbox/mapbox-gl-native/pull/14847/files/d0f219dde6c20ecea9c4242fe56161550131ff74#diff-9066f832688a89e2efb34836fe4d9083R140
| if (request) { | ||
| // Remove the callback function because the AsyncRequest was | ||
| // canceled and we are no longer interested in the result. | ||
| request->callback = {}; |
There was a problem hiding this comment.
maybe a request->cancel() would be a cleaner interface
There was a problem hiding this comment.
Can you also add a MBGL_VERIFY_THREAD to this object? We need to make sure it lives on the main thread.
There was a problem hiding this comment.
The lifetime of this request object is managed by the unique_ptr that is returned here I thought - https://github.com/mapbox/mapbox-gl-native/blob/node10_support/platform/node/src/node_map.cpp#L1240
| auto callback = std::move(request->callback); | ||
| request->callback = {}; | ||
| if (!callback) { | ||
| request->unref(); |
There was a problem hiding this comment.
Just to make this code safer, does it work if you create a handle and unref a the beginning of the code? So when the handle goes out of scope we no longer have refs.
There was a problem hiding this comment.
Perhaps my suggestion is overkill :-)
There was a problem hiding this comment.
I am not sure how you would want this done honestly, could you say more if you want to change this?
| @@ -52,7 +58,9 @@ void NodeRequest::HandleCallback(const Nan::FunctionCallbackInfo<v8::Value>& inf | |||
|
|
|||
| // Move out of the object so callback() can only be fired once. | |||
There was a problem hiding this comment.
Actually I think it will make the refcounter negative on double invocation.
There was a problem hiding this comment.
I am not sure what you mean by this?
There was a problem hiding this comment.
if we call the callback twice by mistake on the Javascript side...
| Nan::AsyncResource res("mbgl:execute"); | ||
| res.runInAsyncScope(Nan::To<v8::Object>(target->handle()->GetInternalField(1)).ToLocalChecked(), "request", 1, argv); | ||
| void NodeRequest::unref() { | ||
| Nan::HandleScope scope; |
There was a problem hiding this comment.
Why is this scope needed?
There was a problem hiding this comment.
I believe it is needed because asyncResource is a resource managed by node and we need to make sure that its removed as well from the memory managed by node.
|
@flippmoke @tmpsantos what are next steps here? |
| @@ -52,7 +58,9 @@ void NodeRequest::HandleCallback(const Nan::FunctionCallbackInfo<v8::Value>& inf | |||
|
|
|||
| // Move out of the object so callback() can only be fired once. | |||
There was a problem hiding this comment.
if we call the callback twice by mistake on the Javascript side...
Good to merge IMO. |
|
@tmpsantos not sure why the status hasn't passed here? |
We recently added 2 new CI bots for running render test. |
flippmoke
force-pushed
the
node10_support
branch
from
d0f219d to
3f6b244
Compare
6 participants
Might solve #12252