src: use symbol to store `AsyncWrap` resource by addaleax · Pull Request #31745 · nodejs/node · GitHub

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

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

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

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

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

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.

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

LGTM, with one non-blocking question.

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

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 <admin@stephenbelanger.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

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 <admin@stephenbelanger.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

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 <admin@stephenbelanger.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

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 <admin@stephenbelanger.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

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

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.

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

Comment thread lib/internal/async_hooks.js Outdated

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 outstanding review comments, 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 outstanding review comments, 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 outstanding review comments, 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

Comment thread src/async_wrap.cc

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

                 static void EmitPromiseResolve(Environment* env, double async_id);
-                void EmitDestroy();
+                void EmitDestroy(bool from_gc = false);

src: use symbol to store `AsyncWrap` resource by addaleax · Pull Request #31745 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: use symbol to store AsyncWrap resource - #31745

src: use symbol to store AsyncWrap resource#31745addaleax wants to merge 2 commits intonodejs:masternodejs/node:masterfrom addaleax:async-hooks-resource-symboladdaleax/node:async-hooks-resource-symbolCopy head branch name to clipboard

Conversation

addaleax commented Feb 12, 2020

Checklist

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 13, 2020

Uh oh!

nodejs-github-bot commented Feb 23, 2020

Uh oh!

nodejs-github-bot commented Feb 26, 2020

Uh oh!

addaleax commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 19, 2020

Uh oh!

nodejs-github-bot commented May 19, 2020

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented May 20, 2020

Uh oh!

jasnell May 22, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax May 22, 2020

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 22, 2020

Uh oh!

MylesBorins commented Jul 21, 2020

Uh oh!

Flarna commented Aug 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

src: use symbol to store AsyncWrap resource#31745
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:async-hooks-resource-symbol

addaleax commented Feb 26, 2020 •
edited
Loading