Fix TODOs to address memory leaks by gabrielschulhof · Pull Request #343 · nodejs/node-addon-api

gabrielschulhof · 2018-09-17T02:52:57Z

Fixes: #333

gabrielschulhof · 2018-09-17T14:26:42Z

Finally, I chose to attach the data by creating a property whose key is an anonymous symbol and whose value is an external containing the data. This leaves the napi_wrap() slot free, and will not leave any unsightly anonymous symbols hanging around on an object once we have ubiquitous support for napi_add_finalizer().

gabrielschulhof · 2018-09-17T14:33:23Z

Testing to make sure that the finalizer gets called could be done using assertions made about the output of a coverage tool. Basically, we have to make sure that the line where the data gets deleted gets hit a certain number of times in response to the global.gc() call here.

I manually verified this by adding an fprintf(stderr, ...) to the finalizer and running the thunking_manual test via node --expose-gc test/thunking_manual.js.

To test this on the CI, we need a code coverage build followed by a code coverage run, followed by parsing of the code coverage output so we can make assertions about when the finalizer gets called.

gabrielschulhof · 2018-09-23T16:18:46Z

@mhdawson I've added back the deprecated stuff such that it's behind the new build flag NODE_ADDON_API_DISABLE_DEPRECATED. If such a flag is absent from the build, all deprecated stuff will be available.

gabrielschulhof · 2018-09-26T16:12:11Z

@mhdawson I had forgotten to update the PR to update the documentation. That's done now.

mhdawson · 2018-09-26T19:32:45Z

      'dependencies': ["<!(node -p \"require('../').gyp\")"],
      'cflags': [ '-Werror', '-Wall', '-Wextra', '-Wpedantic', '-Wunused-parameter' ],
      'cflags_cc': [ '-Werror', '-Wall', '-Wextra', '-Wpedantic', '-Wunused-parameter' ],
+      'defines': ['NODE_ADDON_API_DISABLE_DEPRECATED']


Does this mean we won't test the deprecated functions?

Yes, it does mean that.

mhdawson · 2018-09-26T19:39:02Z

@gabrielschulhof this is big enough it would probably be good to spend a few minutes together going through it. Maybe after the N-API meeting on Thursday.

gabrielschulhof · 2018-09-28T22:34:12Z

+  // After defining the class we iterate once more over the property descriptors
+  // and attach the data associated with accessors and instance methods to the
+  // newly created JavaScript class.
+  for (size_t idx = 0; idx < props_count; idx++) {


@mhdawson I realized that it's OK to always attach this data to the JS class. We do not need to afterwards retrieve the instance method and attach the data to it, because we do not need to worry about the scenario where someone "detaches" an instance method from the class' .prototype property. The reason we do not need to worry is that in our implementation of instance methods, we attempt to napi_unwrap() the this parameter in order to retrieve the instance of the native class, so we can then call the corresponding instance method on the native instance. Since the JavaScript class is garbage-collected, no new JavaScript instances can be created, and so no JavaScript objects can arrive via the this property that will successfully napi_unwrap() to give us the native instance. This fails nicely with a napi_object_expected or a napi_invalid_arg status that gets propagated back into JavaScript as an exception.

Fixes: nodejs#333

Coverage can be skipped by adding --disable-deprecated to the npm command line.

mhdawson · 2018-10-01T17:08:17Z

+      PropertyDescriptor::Function(str7, TestFunction),
+#else // NODE_ADDON_API_DISABLE_DEPRECATED
      PropertyDescriptor::Function(env, str7, TestFunction),
+#endif // !NODE_ADDON_API_DISABLE_DEPRECATED


I think we want to test both the deprecated and non-deprecated so this should just be #ifndef, #endif as opposed to using #else ? Same goes to all of the other places as well.

gabrielschulhof · 2018-10-01T18:11:32Z

@mhdawson I split the deprecated tests into their own test at test/object/object_deprecated.{cc,js} and modified the build to conditionally include test/object/object_deprecated.cc. The test runs unconditionally but short-circuits successfully if binding.object_deprecated is absent.

mhdawson · 2018-10-01T21:02:40Z

Looks good other than the comment about testing should test both deprecated and non-deprecated versions as opposed to one or the other. Looks like all of the other issues we discussed in person are addressed and I'll sign of land once the test ifdefs are fixed up.

gabrielschulhof · 2018-10-01T21:12:49Z

@mhdawson I fixed that. Now, when you run npm test it runs both the deprecated and the non-deprecated tests, and when you run npm test --disable-deprecated it only runs the non-deprecated tests.

mhdawson

LGTM

PR-URL: #343 Fixes: #333 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson · 2018-10-02T14:35:11Z

Landed as 917bd60

PR-URL: nodejs/node-addon-api#343 Fixes: nodejs/node-addon-api#333 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gabrielschulhof force-pushed the fix-leak-symbol branch 3 times, most recently from d65616c to 96665dd Compare September 17, 2018 14:19

gabrielschulhof commented Sep 17, 2018

View reviewed changes

Comment thread napi.h

gabrielschulhof force-pushed the fix-leak-symbol branch 2 times, most recently from c55522e to 3aac9eb Compare September 18, 2018 21:23

mhdawson mentioned this pull request Sep 18, 2018

Next release #347

Closed

6 tasks

gabrielschulhof force-pushed the fix-leak-symbol branch 2 times, most recently from 3b36403 to d7143a5 Compare September 23, 2018 15:41

gabrielschulhof force-pushed the fix-leak-symbol branch 2 times, most recently from a5e4e04 to 791d161 Compare September 26, 2018 15:53

gabrielschulhof force-pushed the fix-leak-symbol branch from 791d161 to 7f0a628 Compare September 26, 2018 16:13

mhdawson reviewed Sep 26, 2018

View reviewed changes

gabrielschulhof commented Sep 28, 2018

View reviewed changes

Gabriel Schulhof added 6 commits September 28, 2018 18:46

Remove TODOs by fixing memory leaks

fe15c7f

Fixes: nodejs#333

Include tests covering deprecated APIs by default

8166ed1

Coverage can be skipped by adding --disable-deprecated to the npm command line.

Add Napi::Object parameter for completeness

9cbbf8f

Add comment to remind about using napi_add_finalizer()

5d164f5

copy property descriptors in DefineClass

0d8609a

Update comment regarding preprocessor flag

b2a76ab

gabrielschulhof force-pushed the fix-leak-symbol branch from 66f9586 to b2a76ab Compare September 28, 2018 22:46

mhdawson reviewed Oct 1, 2018

View reviewed changes

split deprecated object tests into their own toplevel test

dcb33cd

gabrielschulhof force-pushed the fix-leak-symbol branch from bdf90ad to dcb33cd Compare October 1, 2018 18:13

mhdawson approved these changes Oct 2, 2018

View reviewed changes

mhdawson pushed a commit that referenced this pull request Oct 2, 2018

src: remove TODOs by fixing memory leaks

917bd60

PR-URL: #343 Fixes: #333 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson closed this Oct 2, 2018

gabrielschulhof deleted the fix-leak-symbol branch October 2, 2018 18:03

mhdawson mentioned this pull request Oct 19, 2018

Fatal error with ObjectWrap since 1.5.0 #372

Closed

legendecas mentioned this pull request Mar 22, 2021

doc: unambiguously mark deprecated signatures #942

Closed

JckXia mentioned this pull request Apr 30, 2021

[Tests] Add test coverage for PropertyDescriptors #986

Closed

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022

src: remove TODOs by fixing memory leaks

bde57ad

PR-URL: nodejs/node-addon-api#343 Fixes: nodejs/node-addon-api#333 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022

src: remove TODOs by fixing memory leaks

3996390

PR-URL: nodejs/node-addon-api#343 Fixes: nodejs/node-addon-api#333 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022

src: remove TODOs by fixing memory leaks

698700d

PR-URL: nodejs/node-addon-api#343 Fixes: nodejs/node-addon-api#333 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023

src: remove TODOs by fixing memory leaks

0bd2b63

PR-URL: nodejs/node-addon-api#343 Fixes: nodejs/node-addon-api#333 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Uh oh!

Conversation

gabrielschulhof commented Sep 17, 2018

Uh oh!

Uh oh!

gabrielschulhof commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielschulhof commented Sep 17, 2018

Uh oh!

gabrielschulhof commented Sep 23, 2018

Uh oh!

gabrielschulhof commented Sep 26, 2018

Uh oh!

mhdawson Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Sep 26, 2018

Uh oh!

gabrielschulhof Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Oct 1, 2018

Uh oh!

gabrielschulhof commented Oct 1, 2018

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Oct 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gabrielschulhof commented Sep 17, 2018 •

edited

Loading

gabrielschulhof Sep 28, 2018 •

edited

Loading

gabrielschulhof commented Oct 1, 2018 •

edited

Loading