-
Notifications
You must be signed in to change notification settings - Fork 459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tsfn: fix crash on releasing tsfn #532
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@mhdawson hi, how do we run CI for this PR? |
@legendecas the jobs are here: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/ What'd we like to do is to run node-test-node-addon-api-LTS versions, but I don't have it setup to do that. Instead you'll need to run https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/ on 8.x, 10.x 112.x and 13.x (master) using the NODEJS_MAJOR_VERSION selection box to choose the one for each run. You also need to update the GIT_REPO and GIT_BRANCH to point to the repo for the repo and branch for the PR. Assuming you have access to start jobs in the Node.js CI you should be able to do that and if you do please post the links in the issues along with the pass/fail status. |
@legendecas, depending on the change I also often just run the test locally as part of landing the change, with the fallback that an cross platform problems will show un up in the nightly run. |
Given that I don't expect failures on one platform but not another I can go ahead and try to land. @legendecas I assume you've run the tests locally and they all pass? |
Yes, it's all passed. ⋊> ~/D/g/node-addon-api on tsfn ◦ npm test 09:55:40
> node-addon-api@1.7.1 pretest /Users/lucas/Developer/github/node-addon-api
> node-gyp rebuild -C test
CC(target) Release/obj.target/nothing/../src/nothing.o
LIBTOOL-STATIC Release/nothing.a
CXX(target) Release/obj.target/binding/arraybuffer.o
CXX(target) Release/obj.target/binding/asynccontext.o
CXX(target) Release/obj.target/binding/asyncworker.o
CXX(target) Release/obj.target/binding/asyncworker-persistent.o
CXX(target) Release/obj.target/binding/basic_types/array.o
CXX(target) Release/obj.target/binding/basic_types/boolean.o
CXX(target) Release/obj.target/binding/basic_types/number.o
CXX(target) Release/obj.target/binding/basic_types/value.o
CXX(target) Release/obj.target/binding/bigint.o
CXX(target) Release/obj.target/binding/binding.o
CXX(target) Release/obj.target/binding/buffer.o
CXX(target) Release/obj.target/binding/callbackscope.o
CXX(target) Release/obj.target/binding/dataview/dataview.o
CXX(target) Release/obj.target/binding/dataview/dataview_read_write.o
CXX(target) Release/obj.target/binding/error.o
CXX(target) Release/obj.target/binding/external.o
CXX(target) Release/obj.target/binding/function.o
CXX(target) Release/obj.target/binding/handlescope.o
CXX(target) Release/obj.target/binding/memory_management.o
CXX(target) Release/obj.target/binding/name.o
CXX(target) Release/obj.target/binding/object/delete_property.o
CXX(target) Release/obj.target/binding/object/get_property.o
CXX(target) Release/obj.target/binding/object/has_own_property.o
CXX(target) Release/obj.target/binding/object/has_property.o
CXX(target) Release/obj.target/binding/object/object.o
CXX(target) Release/obj.target/binding/object/set_property.o
CXX(target) Release/obj.target/binding/promise.o
CXX(target) Release/obj.target/binding/threadsafe_function/threadsafe_function_ptr.o
CXX(target) Release/obj.target/binding/threadsafe_function/threadsafe_function.o
CXX(target) Release/obj.target/binding/typedarray.o
CXX(target) Release/obj.target/binding/objectwrap.o
CXX(target) Release/obj.target/binding/objectreference.o
CXX(target) Release/obj.target/binding/version_management.o
CXX(target) Release/obj.target/binding/thunking_manual.o
CXX(target) Release/obj.target/binding/object/object_deprecated.o
SOLINK_MODULE(target) Release/binding.node
CXX(target) Release/obj.target/binding_noexcept/arraybuffer.o
CXX(target) Release/obj.target/binding_noexcept/asynccontext.o
CXX(target) Release/obj.target/binding_noexcept/asyncworker.o
CXX(target) Release/obj.target/binding_noexcept/asyncworker-persistent.o
CXX(target) Release/obj.target/binding_noexcept/basic_types/array.o
CXX(target) Release/obj.target/binding_noexcept/basic_types/boolean.o
CXX(target) Release/obj.target/binding_noexcept/basic_types/number.o
CXX(target) Release/obj.target/binding_noexcept/basic_types/value.o
CXX(target) Release/obj.target/binding_noexcept/bigint.o
CXX(target) Release/obj.target/binding_noexcept/binding.o
CXX(target) Release/obj.target/binding_noexcept/buffer.o
CXX(target) Release/obj.target/binding_noexcept/callbackscope.o
CXX(target) Release/obj.target/binding_noexcept/dataview/dataview.o
CXX(target) Release/obj.target/binding_noexcept/dataview/dataview_read_write.o
CXX(target) Release/obj.target/binding_noexcept/error.o
CXX(target) Release/obj.target/binding_noexcept/external.o
CXX(target) Release/obj.target/binding_noexcept/function.o
CXX(target) Release/obj.target/binding_noexcept/handlescope.o
CXX(target) Release/obj.target/binding_noexcept/memory_management.o
CXX(target) Release/obj.target/binding_noexcept/name.o
CXX(target) Release/obj.target/binding_noexcept/object/delete_property.o
CXX(target) Release/obj.target/binding_noexcept/object/get_property.o
CXX(target) Release/obj.target/binding_noexcept/object/has_own_property.o
CXX(target) Release/obj.target/binding_noexcept/object/has_property.o
CXX(target) Release/obj.target/binding_noexcept/object/object.o
CXX(target) Release/obj.target/binding_noexcept/object/set_property.o
CXX(target) Release/obj.target/binding_noexcept/promise.o
CXX(target) Release/obj.target/binding_noexcept/threadsafe_function/threadsafe_function_ptr.o
CXX(target) Release/obj.target/binding_noexcept/threadsafe_function/threadsafe_function.o
CXX(target) Release/obj.target/binding_noexcept/typedarray.o
CXX(target) Release/obj.target/binding_noexcept/objectwrap.o
CXX(target) Release/obj.target/binding_noexcept/objectreference.o
CXX(target) Release/obj.target/binding_noexcept/version_management.o
CXX(target) Release/obj.target/binding_noexcept/thunking_manual.o
CXX(target) Release/obj.target/binding_noexcept/object/object_deprecated.o
SOLINK_MODULE(target) Release/binding_noexcept.node
> node-addon-api@1.7.1 test /Users/lucas/Developer/github/node-addon-api
> node test
Starting test suite
Running test 'arraybuffer'
Running test 'asynccontext'
Running test 'asyncworker'
Running test 'asyncworker-nocallback'
Running test 'asyncworker-persistent'
Running test 'basic_types/array'
Running test 'basic_types/boolean'
Running test 'basic_types/number'
Running test 'basic_types/value'
Running test 'bigint'
Running test 'buffer'
Running test 'callbackscope'
Running test 'dataview/dataview'
Running test 'dataview/dataview_read_write'
Running test 'error'
Running test 'external'
Running test 'function'
Running test 'handlescope'
Running test 'memory_management'
Running test 'name'
Running test 'object/delete_property'
Running test 'object/get_property'
Running test 'object/has_own_property'
Running test 'object/has_property'
Running test 'object/object'
Running test 'object/object_deprecated'
Running test 'object/set_property'
Running test 'promise'
Running test 'threadsafe_function/threadsafe_function_ptr'
Running test 'threadsafe_function/threadsafe_function'
Running test 'typedarray'
Running test 'typedarray-bigint'
Running test 'objectwrap'
Running test 'objectreference'
Running test 'version_management'
All tests passed!
⋊> ~/D/g/node-addon-api on tsfn ◦ echo $status 09:56:41
0 |
Landed as 0b4f3a5 |
Refs: nodejs/node-addon-api#531 PR-URL: nodejs/node-addon-api#532 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Refs: nodejs/node-addon-api#531 PR-URL: nodejs/node-addon-api#532 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Refs: nodejs/node-addon-api#531 PR-URL: nodejs/node-addon-api#532 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Refs: nodejs/node-addon-api#531 PR-URL: nodejs/node-addon-api#532 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
napi_threadsafe_function*
returned from out param ofnapi_create_threadsafe_function
should not be deleted outside of node core.Related: #531