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

domain: avoid circular memory references #25993

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Feb 7, 2019

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 iddomain 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
@nodejs-github-bot 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
@ChALkeR
Copy link
Member

ChALkeR commented Feb 7, 2019

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?

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

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

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

@addaleax
Copy link
Member Author

addaleax commented Feb 7, 2019

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

@ChALkeR
Copy link
Member

ChALkeR commented Feb 7, 2019

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

src/node_util.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

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

lib/domain.js Show resolved Hide resolved
@addaleax
Copy link
Member Author

addaleax commented Feb 7, 2019

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.

@refack
Copy link
Contributor

refack commented Feb 7, 2019

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.

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

@vdeturckheim vdeturckheim left a comment

Choose a reason for hiding this comment

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

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.

@addaleax
Copy link
Member Author

addaleax commented Feb 8, 2019

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.

@addaleax
Copy link
Member Author

addaleax commented Feb 8, 2019

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

@@ -53,20 +57,22 @@ const asyncHook = createHook({
init(asyncId, type, triggerAsyncId, resource) {
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]);
Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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…

@refack
Copy link
Contributor

refack commented Feb 8, 2019

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 8, 2019
@addaleax
Copy link
Member Author

addaleax commented Feb 12, 2019

@addaleax addaleax deleted the domain-leak branch February 12, 2019 20:13
addaleax added a commit that referenced this pull request Feb 13, 2019
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax added a commit that referenced this pull request Feb 13, 2019
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
@avaly
Copy link

avaly commented Feb 15, 2019

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

@abraxxas
Copy link

Any update if this will be backported to LTS?

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request May 17, 2019
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request May 17, 2019
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins
Copy link
Contributor

Backport opened in #27749

MylesBorins pushed a commit that referenced this pull request May 20, 2019
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 20, 2019
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
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++. domain Issues and PRs related to the domain subsystem. memory Issues and PRs related to the memory management or memory footprint.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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