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

src: add AsyncWorker destruction suppression #407

Conversation

gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Dec 7, 2018

Add method SuppressDestruct() to AsyncWorker, which will cause an
instance of the class to remain allocated even after the OnOK
callback fires. Such an instance must be explicitly delete-ed from
user code.

Re: #231
Re: nodejs/abi-stable-node#353

@gabrielschulhof
Copy link
Contributor Author

@ebickle PTAL

@gabrielschulhof gabrielschulhof deleted the suppress-async-worker-destruct branch December 27, 2018 18:01
@gabrielschulhof gabrielschulhof restored the suppress-async-worker-destruct branch December 28, 2018 20:27
@gabrielschulhof
Copy link
Contributor Author

Whoops! I accidentally closed this.

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

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM

@ebickle
Copy link

ebickle commented Jan 24, 2019

LGTM

@NickNaso
Copy link
Member

NickNaso commented Feb 8, 2019

@gabrielschulhof Does it fail on every version of Node.js or one in the specific?

@gabrielschulhof
Copy link
Contributor Author

@NickNaso all.

@gabrielschulhof
Copy link
Contributor Author

OK. This is weird. It looks like OSX merges data symbols from different .node files!

Running test 'asyncworker-persistent'
Assigning current_worker@0x102b51d00: 0x1029291c0
0x1029291c0: Setting _suppress_destruct to true
Assigning current_worker@0x102b51d00: 0x10292bad0
0x10292bad0: Setting _suppress_destruct to true

The location of current_worker (the address following the @) is the same for both the has-exceptions and no-exceptions module!

@gabrielschulhof
Copy link
Contributor Author

New CI:

version job
v6.x https://ci.nodejs.org/job/node-test-node-addon-api/1556/

@gabrielschulhof
Copy link
Contributor Author

Still dies on OSX 😕

@gabrielschulhof
Copy link
Contributor Author

@mhdawson @NickNaso the moral of the story is that we need to add the condition

['OS!="windows"', {
  'cflags+': ['-fvisibility=hidden'],
  'xcode_settings': {
    'OTHER_CFLAGS': ['-fvisibility=hidden']
  }
}]

to test/binding.gyp to make sure that symbols produced in binding.node and binding_noexcept.node are not exported, otherwise they seem to "overlap" on OSX 😕

Thus, could you please have another look at the PR? The implementation hasn't changed, but I cleaned up the test a little.

@gabrielschulhof
Copy link
Contributor Author

@mhdawson @NickNaso ... and I added the condition to test/binding.gyp.

Add method `SuppressDestruct()` to `AsyncWorker`, which will cause an
instance of the class to remain allocated even after the `OnOK`
callback fires. Such an instance must be explicitly `delete`-ed from
user code.

Re: nodejs#231
Re: nodejs/abi-stable-node#353
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Feb 14, 2019

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

mhdawson commented Mar 4, 2019

Discussed in most recent N-API team meetings. My understanding is that we are going to try out some other options so will mark as request changes until we do that.

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.

Discussed in most recent N-API team meetings. My understanding is that we are going to try out some other options so will mark as request changes until we do that.

@gabrielschulhof
Copy link
Contributor Author

@mhdawson @NickNaso since we have sufficient approval of nodejs/node-gyp#1689 (1 collaborator approval + 7 days) and since it introduces the same change as this PR (to add -fvisibility=hidden on OSX) to the build system, and since changes from node-gyp will take a long time to arrive in all supported Node.js versions, I believe it would be OK to land this PR.

We should also document that we recommend that folks add -fvisibility=hidden to their own binding.gyps if they support OSX. I can open an issue for that.

@NickNaso
Copy link
Member

@gabrielschulhof I'm ok with your idea and I think that we need to update the documentation in the setup section https://github.com/nodejs/node-addon-api/blob/master/doc/setup.md#installation-and-usage.

@mhdawson
Copy link
Member

@gabrielschulhof sounds good to me.

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

mhdawson pushed a commit that referenced this pull request Mar 20, 2019
Add method `SuppressDestruct()` to `AsyncWorker`, which will cause an
instance of the class to remain allocated even after the `OnOK`
callback fires. Such an instance must be explicitly `delete`-ed from
user code.

Re: #231
Re: nodejs/abi-stable-node#353

PR-URL: #407
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
@mhdawson
Copy link
Member

Landed as b0f6b60

@mhdawson mhdawson closed this Mar 20, 2019
@gabrielschulhof gabrielschulhof deleted the suppress-async-worker-destruct branch December 17, 2019 01:36
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
Add method `SuppressDestruct()` to `AsyncWorker`, which will cause an
instance of the class to remain allocated even after the `OnOK`
callback fires. Such an instance must be explicitly `delete`-ed from
user code.

Re: nodejs/node-addon-api#231
Re: nodejs/abi-stable-node#353

PR-URL: nodejs/node-addon-api#407
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
Add method `SuppressDestruct()` to `AsyncWorker`, which will cause an
instance of the class to remain allocated even after the `OnOK`
callback fires. Such an instance must be explicitly `delete`-ed from
user code.

Re: nodejs/node-addon-api#231
Re: nodejs/abi-stable-node#353

PR-URL: nodejs/node-addon-api#407
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
Add method `SuppressDestruct()` to `AsyncWorker`, which will cause an
instance of the class to remain allocated even after the `OnOK`
callback fires. Such an instance must be explicitly `delete`-ed from
user code.

Re: nodejs/node-addon-api#231
Re: nodejs/abi-stable-node#353

PR-URL: nodejs/node-addon-api#407
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
austinli64 added a commit to austinli64/node-addon-api that referenced this pull request May 9, 2023
Add method `SuppressDestruct()` to `AsyncWorker`, which will cause an
instance of the class to remain allocated even after the `OnOK`
callback fires. Such an instance must be explicitly `delete`-ed from
user code.

Re: nodejs/node-addon-api#231
Re: nodejs/abi-stable-node#353

PR-URL: nodejs/node-addon-api#407
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
Add method `SuppressDestruct()` to `AsyncWorker`, which will cause an
instance of the class to remain allocated even after the `OnOK`
callback fires. Such an instance must be explicitly `delete`-ed from
user code.

Re: nodejs/node-addon-api#231
Re: nodejs/abi-stable-node#353

PR-URL: nodejs/node-addon-api#407
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
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.

None yet

4 participants