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: use symbol to store AsyncWrap resource #31745

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

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

@addaleax addaleax requested a review from Qard February 12, 2020 00:26
@nodejs-github-bot 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
lib/internal/async_hooks.js Outdated Show resolved Hide resolved
@addaleax 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
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax 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
Copy link
Member Author

addaleax commented Feb 26, 2020

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.

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label Feb 26, 2020
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 addaleax force-pushed the async-hooks-resource-symbol branch from 64ce34f to a6b337d Compare May 19, 2020 16:26
@addaleax
Copy link
Member Author

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.

@addaleax 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
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Qard Qard left a comment

Choose a reason for hiding this comment

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

LGTM, with one non-blocking question.

src/async_wrap.cc Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

jasnell pushed a commit that referenced this pull request May 22, 2020
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>
@jasnell
Copy link
Member

jasnell commented May 22, 2020

Landed in 6f6bf01

@jasnell jasnell closed this May 22, 2020
codebytere pushed a commit that referenced this pull request Jun 18, 2020
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>
@addaleax addaleax deleted the async-hooks-resource-symbol branch June 19, 2020 13:47
addaleax added a commit to addaleax/node that referenced this pull request Jun 19, 2020
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>
BethGriggs pushed a commit that referenced this pull request Jun 26, 2020
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>
BethGriggs added a commit that referenced this pull request Jun 26, 2020
Notable changes:

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

PR-URL: TBD
@BethGriggs BethGriggs mentioned this pull request Jun 26, 2020
BethGriggs added a commit that referenced this pull request Jun 26, 2020
Notable changes:

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

PR-URL: #34077
@codebytere codebytere mentioned this pull request Jun 28, 2020
BethGriggs added a commit that referenced this pull request Jun 30, 2020
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
Copy link
Contributor

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
Copy link
Member

Flarna commented Aug 20, 2020

Refs: #30959

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.