[v12.x] src: use symbol to store AsyncWrap resource - #33962
Conversation
Sorry, something went wrong.
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>
|
Fwiw, I opened this backport mostly since reports in #33468 indicate that this fixes a memory leak. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
Sorry, something went wrong.
|
@addaleax Any idea why this fixes a memory? |
Sorry, something went wrong.
@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. |
Sorry, something went wrong.
|
@richardlau Not yet – my comment above is a request for fast-tracking this, essentially. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Sorry, something went wrong.
mhdawson
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
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 ? |
Sorry, something went wrong.
|
@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)) |
Sorry, something went wrong.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/32083/ (:yellow_heart:) |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
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>
Use a symbol on the bindings object to store the public resource object,
rather than a v8::Global Persistent. This has several advantages:
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 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