src: use symbol to store `AsyncWrap` resource #31745

addaleax · 2020-02-12T00:26:43Z

Use a symbol on the bindings object to store the public resource object,
rather than a v8::Global Persistent. This has several advantages:

It’s harder to inadvertently create memory leaks this way.
The garbage collector sees the AsyncWrap → resource link like
a regular JS property, and can collect the objects as a group,
even if the resource object should happen to point back to the
AsyncWrap object.
This will make it easier in the future to use owner_symbol for
this purpose, which is generally the direction we should be moving
the async_hooks API into (i.e. using more public objects instead
of letting internal wires stick out).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

lib/internal/async_hooks.js

nodejs-github-bot · 2020-02-13T20:03:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29108/

nodejs-github-bot · 2020-02-23T02:28:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29290/

nodejs-github-bot · 2020-02-26T03:31:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29364/

addaleax · 2020-02-26T18:55:41Z

Looks like un-setting the field during GC isn’t allowed (which is reasonable, I guess). I’ll switch this over to using an internal field after #31960 lands – that should work as well. The big downside there is that it will make merging with owner_symbol harder, not easier.

Use a symbol on the bindings object to store the public resource object, rather than a `v8::Global` Persistent. This has several advantages: - It’s harder to inadvertently create memory leaks this way. The garbage collector sees the `AsyncWrap` → resource link like a regular JS property, and can collect the objects as a group, even if the resource object should happen to point back to the `AsyncWrap` object. - This will make it easier in the future to use `owner_symbol` for this purpose, which is generally the direction we should be moving the `async_hooks` API into (i.e. using more public objects instead of letting internal wires stick out).

addaleax · 2020-05-19T16:58:08Z

I’ve rebased this, with a smaller patch that instead only unsets the resource if not triggered from GC. I think that’s fine, considering that the object won’t be accessed anymore anyway if EmitDestroy() is caued by GC, and it’s more in line with the original intention of the PR, which included making this more similar to owner_symbol.

nodejs-github-bot · 2020-05-19T16:58:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31415/

Qard

LGTM, with one non-blocking question.

src/async_wrap.cc

nodejs-github-bot · 2020-05-20T10:31:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31437/

jasnell · 2020-05-22T00:43:56Z

src/async_wrap.h

@@ -152,7 +152,7 @@ class AsyncWrap : public BaseObject {
  static void EmitAfter(Environment* env, double async_id);
  static void EmitPromiseResolve(Environment* env, double async_id);

-  void EmitDestroy();
+  void EmitDestroy(bool from_gc = false);


non-blocking nit... would prefer an enum here rather than a bool but that can be done in a separate PR

I generally agree with that but here there’s effectively only one call site where passing true ever makes sense, so I don’t think this really falls under “boolean trap”.

Use a symbol on the bindings object to store the public resource object, rather than a `v8::Global` Persistent. This has several advantages: - It’s harder to inadvertently create memory leaks this way. The garbage collector sees the `AsyncWrap` → resource link like a regular JS property, and can collect the objects as a group, even if the resource object should happen to point back to the `AsyncWrap` object. - This will make it easier in the future to use `owner_symbol` for this purpose, which is generally the direction we should be moving the `async_hooks` API into (i.e. using more public objects instead of letting internal wires stick out). PR-URL: #31745 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

jasnell · 2020-05-22T00:46:23Z

Landed in 6f6bf01

Use a symbol on the bindings object to store the public resource object, rather than a `v8::Global` Persistent. This has several advantages: - It’s harder to inadvertently create memory leaks this way. The garbage collector sees the `AsyncWrap` → resource link like a regular JS property, and can collect the objects as a group, even if the resource object should happen to point back to the `AsyncWrap` object. - This will make it easier in the future to use `owner_symbol` for this purpose, which is generally the direction we should be moving the `async_hooks` API into (i.e. using more public objects instead of letting internal wires stick out). PR-URL: #31745 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

Use a symbol on the bindings object to store the public resource object, rather than a `v8::Global` Persistent. This has several advantages: - It’s harder to inadvertently create memory leaks this way. The garbage collector sees the `AsyncWrap` → resource link like a regular JS property, and can collect the objects as a group, even if the resource object should happen to point back to the `AsyncWrap` object. - This will make it easier in the future to use `owner_symbol` for this purpose, which is generally the direction we should be moving the `async_hooks` API into (i.e. using more public objects instead of letting internal wires stick out). PR-URL: nodejs#31745 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

Use a symbol on the bindings object to store the public resource object, rather than a `v8::Global` Persistent. This has several advantages: - It’s harder to inadvertently create memory leaks this way. The garbage collector sees the `AsyncWrap` → resource link like a regular JS property, and can collect the objects as a group, even if the resource object should happen to point back to the `AsyncWrap` object. - This will make it easier in the future to use `owner_symbol` for this purpose, which is generally the direction we should be moving the `async_hooks` API into (i.e. using more public objects instead of letting internal wires stick out). PR-URL: #31745 Backport-PR-URL: #33962 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

Notable changes: - deps: V8: backport fb26d0bb1835 (Matheus Marchini) [#33573](#33573) - src: use symbol to store `AsyncWrap` resource (Anna Henningsen) [#31745](#31745) PR-URL: TBD

Notable changes: - deps: V8: backport fb26d0bb1835 (Matheus Marchini) [#33573](#33573) - src: use symbol to store `AsyncWrap` resource (Anna Henningsen) [#31745](#31745) PR-URL: #34077

MylesBorins · 2020-07-21T20:14:45Z

This change landing on v12.18.2 resulted in breakages in the glob-watcher module being used internally by gulp. The breakages appear to be related to an out of date dependency on chokidar which in turn depends on an out of date version of fsevents. While this did break in a semver-patch, it is not clear that this specific change caused the breakage as opposed to making an existing breakage more obvious by fixing something else.

At the moment I don't think there is any need to revert, but wanted to document research here.

Flarna · 2020-08-20T08:11:38Z

Refs: #30959

It's somehow a followup and fixed an issue introduced so keep them linked.

addaleax requested a review from Qard February 12, 2020 00:26

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 12, 2020

addaleax added the async_wrap label Feb 12, 2020

Qard approved these changes Feb 12, 2020

View reviewed changes

devnexen approved these changes Feb 12, 2020

View reviewed changes

lundibundi approved these changes Feb 12, 2020

View reviewed changes

lib/internal/async_hooks.js Outdated Show resolved Hide resolved

legendecas approved these changes Feb 12, 2020

View reviewed changes

benjamingr approved these changes Feb 12, 2020

View reviewed changes

jasnell approved these changes Feb 12, 2020

View reviewed changes

JungMinu approved these changes Feb 13, 2020

View reviewed changes

cjihrig approved these changes Feb 13, 2020

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 13, 2020

addaleax mentioned this pull request Feb 26, 2020

Investigate flaky test-http2-ping-settings-heapdump #31962

Closed

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 26, 2020

addaleax added the blocked PRs that are blocked by other issues or PRs. label Feb 26, 2020

fhinkel approved these changes Apr 21, 2020

View reviewed changes

Qard approved these changes Apr 21, 2020

View reviewed changes

addaleax mentioned this pull request May 19, 2020

TLSWrap objects grow with HTTP keep-alive enabled until out of memory error occurs #33468

Closed

addaleax added 2 commits May 19, 2020 14:56

fixup! src: use symbol to store AsyncWrap resource

a6b337d

addaleax force-pushed the async-hooks-resource-symbol branch from 64ce34f to a6b337d Compare May 19, 2020 16:26

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels May 19, 2020

Qard approved these changes May 19, 2020

View reviewed changes

src/async_wrap.cc Show resolved Hide resolved

benjamingr approved these changes May 19, 2020

View reviewed changes

Flarna approved these changes May 19, 2020

View reviewed changes

jasnell reviewed May 22, 2020

View reviewed changes

jasnell approved these changes May 22, 2020

View reviewed changes

jasnell closed this May 22, 2020

addaleax deleted the async-hooks-resource-symbol branch June 19, 2020 13:47

addaleax mentioned this pull request Jun 19, 2020

[v12.x] src: use symbol to store AsyncWrap resource #33962

Closed

addaleax added the backport-open-v12.x label Jun 19, 2020

BethGriggs mentioned this pull request Jun 26, 2020

v12.18.2 proposal #34077

Merged

codebytere mentioned this pull request Jun 28, 2020

v14.5.0 proposal #34093

Merged

MylesBorins mentioned this pull request Jul 21, 2020

deps: V8: cherry-pick eec10a2fd8fa #33778

Closed

addaleax added backported-to-v12.x and removed backport-open-v12.x labels Sep 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: use symbol to store `AsyncWrap` resource #31745

src: use symbol to store `AsyncWrap` resource #31745

addaleax commented Feb 12, 2020

nodejs-github-bot commented Feb 13, 2020

nodejs-github-bot commented Feb 23, 2020

nodejs-github-bot commented Feb 26, 2020

addaleax commented Feb 26, 2020 •

edited

Loading

addaleax commented May 19, 2020

nodejs-github-bot commented May 19, 2020

Qard left a comment

nodejs-github-bot commented May 20, 2020

jasnell May 22, 2020

addaleax May 22, 2020

jasnell commented May 22, 2020

MylesBorins commented Jul 21, 2020

Flarna commented Aug 20, 2020

src: use symbol to store AsyncWrap resource #31745

src: use symbol to store AsyncWrap resource #31745

Conversation

addaleax commented Feb 12, 2020

Checklist

nodejs-github-bot commented Feb 13, 2020

nodejs-github-bot commented Feb 23, 2020

nodejs-github-bot commented Feb 26, 2020

addaleax commented Feb 26, 2020 • edited Loading

addaleax commented May 19, 2020

nodejs-github-bot commented May 19, 2020

Qard left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 20, 2020

jasnell May 22, 2020

Choose a reason for hiding this comment

addaleax May 22, 2020

Choose a reason for hiding this comment

jasnell commented May 22, 2020

MylesBorins commented Jul 21, 2020

Flarna commented Aug 20, 2020

src: use symbol to store `AsyncWrap` resource #31745

src: use symbol to store `AsyncWrap` resource #31745

addaleax commented Feb 26, 2020 •

edited

Loading