Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Aug 29, 2019

napi_threadsafe_function* returned from out param of napi_create_threadsafe_function should not be deleted outside of node core.

Related: #531

  • npm test passed
  • test added

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@legendecas
Copy link
Member Author

@mhdawson hi, how do we run CI for this PR?

@mhdawson
Copy link
Member

mhdawson commented Sep 3, 2019

@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.

@mhdawson
Copy link
Member

mhdawson commented Sep 3, 2019

@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.

@mhdawson
Copy link
Member

mhdawson commented Sep 3, 2019

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?

@legendecas
Copy link
Member Author

legendecas commented Sep 4, 2019

Yes, it's all passed.

> ~/D/g/node-addon-api on tsfn ◦ npm test                                                                     09:55:40

> [email protected] 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

> [email protected] 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

mhdawson pushed a commit that referenced this pull request Sep 4, 2019
Refs: #531

PR-URL: #532
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

mhdawson commented Sep 4, 2019

Landed as 0b4f3a5

@mhdawson mhdawson closed this Sep 4, 2019
@legendecas legendecas deleted the tsfn branch September 4, 2019 15:28
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
Refs: nodejs/node-addon-api#531

PR-URL: nodejs/node-addon-api#532
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
Refs: nodejs/node-addon-api#531

PR-URL: nodejs/node-addon-api#532
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
Refs: nodejs/node-addon-api#531

PR-URL: nodejs/node-addon-api#532
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
Refs: nodejs/node-addon-api#531

PR-URL: nodejs/node-addon-api#532
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants