domain: avoid circular memory references by addaleax · Pull Request #25993 · nodejs/node · GitHub

src: add WeakReference utility

Add a simple WeakReference utility that we can use until the
language provides something on its own.

domain: avoid circular memory references

Avoid circular references that the JS engine cannot see through
because it involves an async id ⇒ domain link.

Using weak references is not a great solution, because it increases
the domain module’s dependency on internals and the added calls into
C++ may affect performance, but it seems like the least bad one.

Fixes: #23862

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Add a simple `WeakReference` utility that we can use until the language provides something on its own.

Avoid circular references that the JS engine cannot see through because it involves an `async id` ⇒ `domain` link. Using weak references is not a great solution, because it increases the domain module’s dependency on internals and the added calls into C++ may affect performance, but it seems like the least bad one. Fixes: nodejs#23862

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2539/pipeline

This approach looks fine to me, given the current situation.

How does the memory profile for this look like? E.g., taken the example from #23862 as-is, and hammering it with GET requests, how high would the memory usage be with and without domains usage on the server side?

LGTM as much as I don't like it. At least it fixes a bad bug.

@ChALkeR What I’m getting is about 8 kB per request with that example before this patch, and flat memory usage afterwards.

@addaleax

and flat memory usage afterwards.

That is what I was expecting, but I was curious about the exact memory usage profile with/without domains code on the server logic side (with this patch enabled) for the same requests pattern.

Anyway, this change looks good to me in approach and code-wise. I have not tested this locally (yet?), but this should be good once CI passes.

I'm not sure I understand the API of WeakReference. Could you write a cctest for it?

Could you write a cctest for it?

I don’t think a cctest makes sense, but I could write a regular test if you feel that it’s important.

I could write a regular test if you feel that it’s important.

If we want to reuse this is other code path I think we should cover the API.

LGTM. Technically this would mean that CLS-like implementation based on Async Hooks should be rewriten ovev domain as there is no mem leak anymore here.

I could write a regular test if you feel that it’s important.

If we want to reuse this is other code path I think we should cover the API.

@refack I think we want to avoid that as much as possible. I’ve added a test, though.

LGTM. Technically this would mean that CLS-like implementation based on Async Hooks should be rewriten ovev domain as there is no mem leak anymore here.

@vdeturckheim No, that’s not necessarily true. The problem here is specific to domains, because they (needlessly?) keep references to objects that they track.

Ideally, we’d revert this change and remove domain.members – I’ll propose this once this PR lands, but I think it would be semver-major.

(That’s doesn’t mean that it’s not easy to run into memory leaks when using async_hooks, but that’s a fundamental design flaw and a much bigger issue.)

Is it conceivable that user code tampers with process.domain?

What would happen if I set process.domain = {}?

I don’t think the domain code is all that tampering-proof to being with…

I’m also not sure what we could do in that case.

Technically not necessary: Persistent<T>::Get() will return an empty handle, causing ReturnValue<T>::Set() to default to undefined.

Thought about it, and I think I’d prefer an explicit variant here, so that it’s a bit more obvious what happens when the object has been GC’ed…

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

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

Add a simple `WeakReference` utility that we can use until the language provides something on its own. PR-URL: #25993 Fixes: #23862 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

Avoid circular references that the JS engine cannot see through because it involves an `async id` ⇒ `domain` link. Using weak references is not a great solution, because it increases the domain module’s dependency on internals and the added calls into C++ may affect performance, but it seems like the least bad one. PR-URL: #25993 Fixes: #23862 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

Is this fix planned to be back-ported to the v10 release?

Any update if this will be backported to LTS?

Add a simple `WeakReference` utility that we can use until the language provides something on its own. PR-URL: nodejs#25993 Fixes: nodejs#23862 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

Avoid circular references that the JS engine cannot see through because it involves an `async id` ⇒ `domain` link. Using weak references is not a great solution, because it increases the domain module’s dependency on internals and the added calls into C++ may affect performance, but it seems like the least bad one. PR-URL: nodejs#25993 Fixes: nodejs#23862 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

Backport opened in #27749

Add a simple `WeakReference` utility that we can use until the language provides something on its own. Backport-PR-URL: #27749 PR-URL: #25993 Fixes: #23862 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

Avoid circular references that the JS engine cannot see through because it involves an `async id` ⇒ `domain` link. Using weak references is not a great solution, because it increases the domain module’s dependency on internals and the added calls into C++ may affect performance, but it seems like the least bad one. Backport-PR-URL: #27749 PR-URL: #25993 Fixes: #23862 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

addaleax added 2 commits February 7, 2019 21:25

src: add WeakReference utility

ebaae77

Add a simple `WeakReference` utility that we can use until the language provides something on its own.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. domain Issues and PRs related to the domain subsystem. util Issues and PRs related to the built-in util module. labels Feb 7, 2019

addaleax mentioned this pull request Feb 7, 2019

Memory leak when a Promise is stored on an Express request that's added to a domain #23862

Closed

ChALkeR added the memory Issues and PRs related to the memory management or memory footprint. label Feb 7, 2019

mcollina approved these changes Feb 7, 2019

View reviewed changes

BridgeAR approved these changes Feb 7, 2019

View reviewed changes

ChALkeR approved these changes Feb 7, 2019

View reviewed changes

ChALkeR reviewed Feb 7, 2019

View reviewed changes

Comment thread src/node_util.cc Outdated

refack reviewed Feb 7, 2019

View reviewed changes

Comment thread lib/domain.js

addaleax removed the util Issues and PRs related to the built-in util module. label Feb 7, 2019

vdeturckheim approved these changes Feb 8, 2019

View reviewed changes

addaleax added 2 commits February 8, 2019 14:03

fixup! src: add WeakReference utility

fa56b35

fixup! domain: avoid circular memory references

30449e8

cjihrig approved these changes Feb 8, 2019

View reviewed changes

bnoordhuis approved these changes Feb 8, 2019

View reviewed changes

refack approved these changes Feb 8, 2019

View reviewed changes

refack added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 8, 2019

addaleax deleted the domain-leak branch February 12, 2019 20:13

garthenweb mentioned this pull request Feb 13, 2019

@sentry/node memory leaks on node 10.13.0 getsentry/sentry-javascript#1762

Closed

2 tasks

targos mentioned this pull request Feb 14, 2019

v11.10.0 proposal #26098

Merged

avaly mentioned this pull request Feb 20, 2019

v10.15.3 proposal #26063

Merged

Hypfer mentioned this pull request Mar 23, 2019

Memory leak in Valetudo? Hypfer/Valetudo#39

Closed

dariodebastiani-roo mentioned this pull request May 3, 2019

v10.16.0 proposal #27514

Merged

BethGriggs added the backport-requested-v10.x label May 10, 2019

MylesBorins mentioned this pull request May 17, 2019

[v10.x] domain: avoid circular memory references #27749

Closed

MylesBorins added backported-to-v10.x and removed backport-requested-v10.x labels May 17, 2019

                   if (process.domain !== null && process.domain !== undefined) {
                     // If this operation is created while in a domain, let's mark it
-                    pairing.set(asyncId, process.domain);
+                    pairing.set(asyncId, process.domain[kWeak]);

+                static void Get(const FunctionCallbackInfo<Value>& args) {
+                  WeakReference* weak_ref = Unwrap<WeakReference>(args.Holder());
+                  Isolate* isolate = args.GetIsolate();
+                  if (!weak_ref->target_.IsEmpty())

domain: avoid circular memory references by addaleax · Pull Request #25993 · nodejs/node · GitHub

Uh oh!

Uh oh!

domain: avoid circular memory references - #25993

domain: avoid circular memory references #25993addaleax wants to merge 4 commits intonodejs:masternodejs/node:masterfrom addaleax:domain-leakaddaleax/node:domain-leakCopy head branch name to clipboard

Conversation

addaleax commented Feb 7, 2019

Checklist

Uh oh!

nodejs-github-bot commented Feb 7, 2019

Uh oh!

ChALkeR commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Feb 7, 2019

Uh oh!

ChALkeR commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

refack commented Feb 7, 2019

Uh oh!

vdeturckheim left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Feb 8, 2019

Uh oh!

addaleax commented Feb 8, 2019

Uh oh!

bnoordhuis Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

refack commented Feb 8, 2019

Uh oh!

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

Uh oh!

Uh oh!

avaly commented Feb 15, 2019

Uh oh!

abraxxas commented Mar 25, 2019

Uh oh!

MylesBorins commented May 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

domain: avoid circular memory references #25993
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:domain-leak

ChALkeR commented Feb 7, 2019 •
edited
Loading

ChALkeR commented Feb 7, 2019 •
edited
Loading

addaleax commented Feb 7, 2019 •
edited
Loading

addaleax commented Feb 12, 2019 •
edited
Loading