Skip to content

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

PR-URL: #31745

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]>
@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. v12.x labels Jun 19, 2020
@addaleax
Copy link
Member Author

Fwiw, I opened this backport mostly since reports in #33468 indicate that this fixes a memory leak.

@addaleax
Copy link
Member Author

Seems confirmed that this fixes the memory leak on v12.x as well.

@nodejs/lts Can I land this on v12.x-staging once CI is good?

@nodejs-github-bot
Copy link
Collaborator

@Flarna
Copy link
Member

Flarna commented Jun 23, 2020

@addaleax Any idea why this fixes a memory?

@richardlau
Copy link
Member

Seems confirmed that this fixes the memory leak on v12.x as well.

@nodejs/lts Can I land this on v12.x-staging once CI is good?

@addaleax has #31745 gone out in a current release? We'd usually want something to live in current for two weeks before it goes out in LTS. Otherwise yes it can land.

@addaleax
Copy link
Member Author

@richardlau Not yet – my comment above is a request for fast-tracking this, essentially.

@addaleax
Copy link
Member Author

@addaleax Any idea why this fixes a memory?

As the commit description says, using v8::Global on objects that themselves can be referenced from JS is a big memory leak footgun, because if the target of the v8::Global refers back in any way to its holder, that’s a circular reference that the GC cannot understand, i.e. a memory leak.

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

I think expediting as requested by @addaleax makes sense as I know of at least one case where people had to revert from 12.18.0 due to the issue, but because 12.18.0 was a security release are under pressure to move up to a version that has the security fixes.

@addaleax were you trying to make sure it goes out in v12.18.2 or were you thinking we should do an earlier releases? I think we want to make sure it makes it into v12.18.2 at the latest but wonder if we should do an earlier release since its a regression that prevents people moving up to a security release.

@BethGriggs thoughts ?

@addaleax
Copy link
Member Author

addaleax commented Jun 25, 2020

@mhdawson You’re right, I hadn’t even considered the security release angle here. I’m not sure how to proceed with that, but it’s certainly a good argument for releasing faster. (Looks like this is a real-world thing, though: #33468 (comment))

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 25, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/32083/ (:yellow_heart:)

@BethGriggs
Copy link
Member

As this may block users from upgrading to the most recent security release, I'm +1 on doing a release sooner containing this PR. I can volunteer to prepare a release for next Tuesday (which would still leave 3 weeks between the next v12.x which I think is reasonable).

/cc @nodejs/releasers

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 <[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]>
@BethGriggs
Copy link
Member

Landed in 30b0339 (v12.x-staging)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

7 participants