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

doc: modified docs to reflect how to invoke gc on Wrapping C++ objects #20269

Closed
wants to merge 76 commits into from
Closed

doc: modified docs to reflect how to invoke gc on Wrapping C++ objects #20269

wants to merge 76 commits into from

Conversation

isurusiri
Copy link
Contributor

Currently the documentation for Wrapping C++ Objects doesn't explain
how to destruct an object by explicitly invoking the garbage collector.
This commit includes a modification to docs which explains how to
invoke garbage collector to delete an object by executing its
destructor.

Fixes: #19876

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. doc Issues and PRs related to the documentations. labels Apr 25, 2018
```
Note that, in this example, the garbage collector is executed explicitly to
properly invoke the wrapper objects' destructor.
Copy link
Member

Choose a reason for hiding this comment

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

object's destructor, no?

Copy link
Member

Choose a reason for hiding this comment

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

I'd also get rid of the superfluous Note that and go with something more like this:

In the example above, the garbage collector is executed explicitly to properly
invoke the wrapper object's destructor.

Copy link
Contributor Author

@isurusiri isurusiri Apr 25, 2018

Choose a reason for hiding this comment

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

@Trott thank you for the feedback. My bad! I'll update the PR with the suggestions 😃

Copy link
Member

Choose a reason for hiding this comment

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

My bad!

Not at all! Your text is fine. I'm just suggesting something that might be an incremental improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

splendid! 😃

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Using the red X because this can't go in.

Calling global.gc() is not a reliable way to trigger processing of weak handles. Use something else if you want deterministic resource management.

The V8 team has stated that you should not depend on any particular behavior because that can change at any time.

@joyeecheung
Copy link
Member

@bnoordhuis I think it's fine to use global.gc() for demonstrating purposes. The commit message needs to be changed to something like "doc: demonstrate how ObjectWrap destructors are invoked during GC", and there can be some docs added saying this is just demonstrating how the destruction works, the GC is supposed to be triggered by the VM - except when running tests to ensure the destructor gets called.

@joyeecheung
Copy link
Member

Also something like

// Allocate a large number of objects to trigger GC
for (let i = 0; i < 1e6; ++i) {
  ({});
}

may be enough to replace global.gc(), but since both our tests and the v8 tests use that builtin, it's probably fine to use it for testing.

@bnoordhuis
Copy link
Member

I disagree. It's documenting and codifying a bad practice. People are going to cargo-cult it because it's in the official docs and all hell will break loose when it stops working.

@@ -595,6 +595,8 @@ class MyObject : public node::ObjectWrap {
static void PlusOne(const v8::FunctionCallbackInfo<v8::Value>& args);
static v8::Persistent<v8::Function> constructor;
double value_;
napi_env env_;
Copy link
Member

@joyeecheung joyeecheung Apr 25, 2018

Choose a reason for hiding this comment

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

The N-API code here are all unnecessary. When I mention the napi tests in #19876 (comment) , I meant the tests demonstrate how it works in the context of napi. But this is the docs for the native addon API, so the code cannot be copied as-is.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 25, 2018

@bnoordhuis How about stressing the allocator like in #20269 (comment) ? Although we could just leave a comment like // After the GC is triggered instead of actually trying to trigger the GC in the example.

@bnoordhuis
Copy link
Member

There are better ways of stress-testing the GC, like --gc_interval / --gc_random_interval=... and --gc_global.

@isurusiri
Copy link
Contributor Author

Appreciate the feedback. I'll update the PR considering above suggestions. It make sense that we should document only the best practices.

@isurusiri
Copy link
Contributor Author

I think I need some help over here. What I found about forcing GC after going through few references available online is that we could do it from C++ like,
while(!V8::IdleNotification()) {};
And also we can use --gc_interval and --gc_global command line options. However global.gc() will force the GC to execute, but this doesn't guarantee that all objects will be swept out, like @bnoordhuis has mentioned above.
Therefore I think it would be enough to mention about the command line options. I need your input on this or any other possible solutions and much appreciated it since I'm new to the community.

@bnoordhuis
Copy link
Member

V8::IdleNotification() is a GC hint, not a command. I think documenting the flags is acceptable as long as it's made clear that they're a) for testing only, and b) provided by V8, not Node.js, and therefore may or may not be available. V8 has a history of removing flags without prior warning.

@isurusiri
Copy link
Contributor Author

Thanks Ben, I'll update the PR.

isurusiri and others added 16 commits April 30, 2018 18:16
Currently the documentation for Wrapping C++ Objects doesn't explain
how to destruct an object by explicitly invoking the garbage collector.
This commit includes a modification to docs which explains how to
invoke garbage collector to delete an object by executing its
destructor.

Fixes: #19876
This commit removes the usage of the deprectated
crypto.DEFAULT_ENCODING.

PR-URL: #20221
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This commit adds a checkMethods function so that it can be reused to
avoid duplicating the same code for instance methods, and static
methods of the Certificate object.

PR-URL: #20224
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Foreground tasks that repost themselves can force the draining loop
to run indefinitely long without giving other tasks chance to run.

This limits the foreground task draining loop to run only the tasks
that were in the tasks queue at the beginning of the loop.

PR-URL: #19987
Fixes: #19937
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
PR-URL: #20271
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #20277
Refs: #19125
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #20244
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: #20243
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
The process("node").mark("gc__stop") is actually
process("node").mark("gc__done"). Use the proper name so that a
developer can use SystemTap to determine the duration of garbage
collection.

PR-URL: #20152
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Also modifies the error messages so they include more information
and are more consistent.

- The message of ERR_SCRIPT_EXECUTION_INTERRUPTED now mentions
  SIGINT and the trailing period is dropped for consistency.
- Added ERR_SCRIPT_EXECUTION_TIMEOUT and include the timeout
  in the message.

PR-URL: #20147
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #20225
Refs: #20039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #20225
Refs: #20039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #20291
Refs: #17156
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20236
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This commit aims to simplify the getFormat function in
diffiehellman.js.

PR-URL: #20246
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit adds a getIntOption function to reduce the code duplicated
for getting the padding, and saltLength options.

PR-URL: #20247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
apapirovski and others added 25 commits April 30, 2018 18:16
When isInsideNodeModules gets called while already processing
another stack trace, V8 will not call prepareStackTrace again.
This used to cause Node.js to just crash — fix it by checking
for expected return type of the stack (Array).

PR-URL: #20266
Fixes: #20258
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: #20356
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Originally wrote this for some work that is going to take a while
longer before it’s ready to be PR’ed, so it seems fine to start
with this on its own.

PR-URL: #20034
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This commit updates the Duplex constructor adding an if statement
checking if options is undefined, and removes the check from the
following three if statements.

PR-URL: #20353
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This commit updates the isFd function to call isUint32 instead of
doing the same thing.

PR-URL: #20330
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
`EVP_PKEY_EC` only covers ANSI X9.62 curves not IETF ones(curve25519
and curve448). This fixes to add support of X25519 in
`tlsSocket.getEphemeralKeyInfo()`.
X448 should be added in the future upgrade to OpenSSL-1.1.1.

PR-URL: #20273
Fixes: #20262
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
When a request with a long payload is received, http2 does
not allow a response that does not process all the incoming
payload. Add a conditional Http2Stream.close call that runs
only if the user hasn't attempted to read the stream.

PR-URL: #20084
Fixes: #20060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Codify types, variable names, and code fragments
checking patterns I've managed to think of.

Some nits were also fixed in passing
(add missing periods, remove extra line breaks etc).

PR-URL: #20390
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #20398
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
To match browser behaviour, intervals should continue being
scheduled even if the user callback threw during execution.

PR-URL: #20002
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
The Buffer#(read|write)U?Int(B|L)E functions should not use a default
value. This is very likely a bug and it was never documented that
way.

Besides that this also improves the tests by adding more tests and by
refactoring them to less code lines.

PR-URL: #20054
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
After passing an HTTP socket, release its associated resources.

PR-URL: #20305
Fixes: #15651
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This switches the url parser from `url.parse()` to the WHATWG URL
parser while keeping `url.parse()` as fallback.

Also add tests for invalid url deprecations and correct hostname
checks.

PR-URL: #20270
Fixes: #19468
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Adjust indentations and fix a typo.

PR-URL: #20315
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
If lines gets skipped, they are marked with three dots. This makes
sure they are better visualized to distinguish them from everything
else.

PR-URL: #20315
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
This improves the error example output by reflecting the current
state. It also makes sure the examples are up to date in general.
`assert.throws` clarified the `ERR_AMBIGUOUS_ARGUMENT` error.

PR-URL: #20313
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
The headers should be handled as all others as well and just indicate
all arguments.

PR-URL: #20312
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
In case there was no ack, the callback would have returned
more than the error as return value. This makes sure that is not
the case anymore.

PR-URL: #20311
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
If `common.expectsError` is used as a callback, it will now also
verify that there is only one argument (the expected error).

PR-URL: #20311
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This removes outdated TODOs and adds a test for invalid input in
`fs.copyFile` and solves a TODO by doing so.

PR-URL: #20319
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
`./configure --xcode` ostensibly let you built with the Xcode IDE but
it has never been tested regularly since its introduction in 2012 and
probably has been broken for years.  Remove it.

PR-URL: #20328
Fixes: #20324
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Fix an issue when an error was emitted by the stream before
`iterator.next()` is called.

PR-URL: #20329
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Remove "(I prefer the former)" from onboarding-extras. Without any
explanation as to why one might prefer A over B, it probably raises more
questions than it answers.

PR-URL: #20393
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Work around nodejs/build#1254, which
effectively breaks stress test CI and CITGM, by avoiding
`std::make_unique` for now.

This workaround should be reverted once that issue is resolved.

Refs: nodejs/build#1254

PR-URL: #20386
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Currently the documentation for Wrapping C++ Objects doesn't explain
how to destruct an object by explicitly invoking the garbage collector.
This commit includes a modification to docs that explain how to force
the garbage collector to clear objects using V8's command line flags.

Fixes: #19876
@isurusiri
Copy link
Contributor Author

Apologies everyone. I think I just messed up the PR.

@isurusiri isurusiri closed this Apr 30, 2018
@AyushG3112
Copy link
Contributor

AyushG3112 commented Apr 30, 2018

@isurusiri try rebasing to upstream/master and force push your branch on origin.

@isurusiri
Copy link
Contributor Author

I fetched from upstream and rebased to upstream/master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.