Skip to content

src: add check against non-weak BaseObjects at process exit#35490

Closed
addaleax wants to merge 2 commits intonodejs:masterfrom
addaleax:no-forgotten-make-weak
Closed

src: add check against non-weak BaseObjects at process exit#35490
addaleax wants to merge 2 commits intonodejs:masterfrom
addaleax:no-forgotten-make-weak

Conversation

@addaleax
Copy link
Member

@addaleax addaleax commented Oct 3, 2020

When a process exits cleanly, i.e. because the event loop ends up without things to wait for, the Node.js objects that are left on the heap should be:

  1. weak, i.e. ready for garbage collection once no longer referenced, or
  2. detached, i.e. scheduled for destruction once no longer referenced, or
  3. an unrefed libuv handle, i.e. does not keep the event loop alive, or
  4. an inactive libuv handle (essentially the same here)

There are a few exceptions to this rule, but generally, if there are C++-backed Node.js objects on the heap that do not fall into the above categories, we may be looking at a potential memory leak. Most likely, the cause is a missing MakeWeak() call on the corresponding object.

In order to avoid this kind of problem, we check the list of BaseObjects for these criteria. In this commit, we only do so when explicitly instructed to or when in debug mode (where --verify-base-objects is always-on).

In particular, this avoids the kinds of memory leak issues that were fixed in the PRs referenced below (hence the blocked label).

Refs: #35488
Refs: #35487
Refs: #35481

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@addaleax
src: add check against non-weak BaseObjects at process exit
ade01e9 
When a process exits cleanly, i.e. because the event loop ends up
without things to wait for, the Node.js objects that are left on
the heap should be:

 1. weak, i.e. ready for garbage collection once no longer
    referenced, or
 2. detached, i.e. scheduled for destruction once no longer
    referenced, or
 3. an unrefed libuv handle, i.e. does not keep the event loop
    alive, or
 4. an inactive libuv handle (essentially the same here)

There are a few exceptions to this rule, but generally,
if there are C++-backed Node.js objects on the heap
that do not fall into the above categories, we may be looking
at a potential memory leak. Most likely, the cause is a missing
`MakeWeak()` call on the corresponding object.

In order to avoid this kind of problem, we check the list
of BaseObjects for these criteria. In this commit, we only do so
when explicitly instructed to or when in debug mode
(where --verify-base-objects is always-on).

In particular, this avoids the kinds of memory leak issues
that were fixed in the PRs referenced below.

Refs: nodejs#35488
Refs: nodejs#35487
Refs: nodejs#35481
@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. blocked PRs that are blocked by other issues or PRs. labels Oct 3, 2020
@addaleax addaleax requested a review from a team as a code owner October 3, 2020 23:20
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 3, 2020

Review requested:

  • @nodejs/http
  • @nodejs/modules
  • @nodejs/net
  • @nodejs/vm

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 3, 2020
addaleax
addaleax commented Oct 3, 2020
View reviewed changes
src/module_wrap.h
// XXX: The garbage collection rules for ModuleWrap are *super* unclear.
// Do these objects ever get GC'd? Are we just okay with leaking them?
return true;
}
Copy link
Member Author

@addaleax addaleax Oct 3, 2020

Choose a reason for hiding this comment

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

I know the bot already pinged the team, but I’d be curious to know if @nodejs/modules knows more about this … especially for ModuleWraps created by the vm module, creating non-GC-able objects can’t be ideal, right…?

Copy link
Member

@joyeecheung joyeecheung Oct 4, 2020

Choose a reason for hiding this comment

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

I thought they were all weak at one point?

mcollina
mcollina approved these changes Oct 4, 2020
View reviewed changes
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

Trott
Trott reviewed Oct 4, 2020
View reviewed changes
@addaleax addaleax mentioned this pull request Oct 4, 2020
Closed
2 tasks
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed blocked PRs that are blocked by other issues or PRs. labels Oct 6, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 6, 2020

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

@addaleax
Copy link
Member Author

addaleax commented Oct 7, 2020

Landed in 40364b1

@addaleax addaleax closed this Oct 7, 2020
@addaleax addaleax deleted the no-forgotten-make-weak branch October 7, 2020 08:55
addaleax added a commit that referenced this pull request Oct 7, 2020
@addaleax
src: add check against non-weak BaseObjects at process exit
40364b1 
When a process exits cleanly, i.e. because the event loop ends up
without things to wait for, the Node.js objects that are left on
the heap should be:

 1. weak, i.e. ready for garbage collection once no longer
    referenced, or
 2. detached, i.e. scheduled for destruction once no longer
    referenced, or
 3. an unrefed libuv handle, i.e. does not keep the event loop
    alive, or
 4. an inactive libuv handle (essentially the same here)

There are a few exceptions to this rule, but generally,
if there are C++-backed Node.js objects on the heap
that do not fall into the above categories, we may be looking
at a potential memory leak. Most likely, the cause is a missing
`MakeWeak()` call on the corresponding object.

In order to avoid this kind of problem, we check the list
of BaseObjects for these criteria. In this commit, we only do so
when explicitly instructed to or when in debug mode
(where --verify-base-objects is always-on).

In particular, this avoids the kinds of memory leak issues
that were fixed in the PRs referenced below.

Refs: #35488
Refs: #35487
Refs: #35481

PR-URL: #35490
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@gengjiawen
Copy link
Member

gengjiawen commented Oct 8, 2020

@addaleax This looks caused macOS build failed in github action

../src/env.cc:1227:22: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
  ForEachBaseObject([this](BaseObject* obj) {
                     ^~~~
1 error generated.

@addaleax
Copy link
Member Author

addaleax commented Oct 8, 2020

@gengjiawen See #35547 :)

@BethGriggs
Copy link
Member

BethGriggs commented Oct 13, 2020

This didn't land cleanly on v14.x, please raise a backport PR if this should land

@BethGriggs BethGriggs mentioned this pull request Oct 13, 2020
Closed
2 tasks
@addaleax addaleax mentioned this pull request Oct 14, 2020
Merged
4 tasks
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@addaleax @joesepi
src: add check against non-weak BaseObjects at process exit
5e130bc 
When a process exits cleanly, i.e. because the event loop ends up
without things to wait for, the Node.js objects that are left on
the heap should be:

 1. weak, i.e. ready for garbage collection once no longer
    referenced, or
 2. detached, i.e. scheduled for destruction once no longer
    referenced, or
 3. an unrefed libuv handle, i.e. does not keep the event loop
    alive, or
 4. an inactive libuv handle (essentially the same here)

There are a few exceptions to this rule, but generally,
if there are C++-backed Node.js objects on the heap
that do not fall into the above categories, we may be looking
at a potential memory leak. Most likely, the cause is a missing
`MakeWeak()` call on the corresponding object.

In order to avoid this kind of problem, we check the list
of BaseObjects for these criteria. In this commit, we only do so
when explicitly instructed to or when in debug mode
(where --verify-base-objects is always-on).

In particular, this avoids the kinds of memory leak issues
that were fixed in the PRs referenced below.

Refs: nodejs#35488
Refs: nodejs#35487
Refs: nodejs#35481

PR-URL: nodejs#35490
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request May 23, 2021
@addaleax
src: add check against non-weak BaseObjects at process exit
733d983 
When a process exits cleanly, i.e. because the event loop ends up
without things to wait for, the Node.js objects that are left on
the heap should be:

 1. weak, i.e. ready for garbage collection once no longer
    referenced, or
 2. detached, i.e. scheduled for destruction once no longer
    referenced, or
 3. an unrefed libuv handle, i.e. does not keep the event loop
    alive, or
 4. an inactive libuv handle (essentially the same here)

There are a few exceptions to this rule, but generally,
if there are C++-backed Node.js objects on the heap
that do not fall into the above categories, we may be looking
at a potential memory leak. Most likely, the cause is a missing
`MakeWeak()` call on the corresponding object.

In order to avoid this kind of problem, we check the list
of BaseObjects for these criteria. In this commit, we only do so
when explicitly instructed to or when in debug mode
(where --verify-base-objects is always-on).

In particular, this avoids the kinds of memory leak issues
that were fixed in the PRs referenced below.

Refs: nodejs#35488
Refs: nodejs#35487
Refs: nodejs#35481

PR-URL: nodejs#35490
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@addaleax addaleax mentioned this pull request May 23, 2021
Closed
targos pushed a commit that referenced this pull request May 25, 2021
@addaleax @targos
src: add check against non-weak BaseObjects at process exit
8ffaf59 
When a process exits cleanly, i.e. because the event loop ends up
without things to wait for, the Node.js objects that are left on
the heap should be:

 1. weak, i.e. ready for garbage collection once no longer
    referenced, or
 2. detached, i.e. scheduled for destruction once no longer
    referenced, or
 3. an unrefed libuv handle, i.e. does not keep the event loop
    alive, or
 4. an inactive libuv handle (essentially the same here)

There are a few exceptions to this rule, but generally,
if there are C++-backed Node.js objects on the heap
that do not fall into the above categories, we may be looking
at a potential memory leak. Most likely, the cause is a missing
`MakeWeak()` call on the corresponding object.

In order to avoid this kind of problem, we check the list
of BaseObjects for these criteria. In this commit, we only do so
when explicitly instructed to or when in debug mode
(where --verify-base-objects is always-on).

In particular, this avoids the kinds of memory leak issues
that were fixed in the PRs referenced below.

Refs: #35488
Refs: #35487
Refs: #35481

PR-URL: #35490
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@addaleax @targos
src: add check against non-weak BaseObjects at process exit
ea311a4 
When a process exits cleanly, i.e. because the event loop ends up
without things to wait for, the Node.js objects that are left on
the heap should be:

 1. weak, i.e. ready for garbage collection once no longer
    referenced, or
 2. detached, i.e. scheduled for destruction once no longer
    referenced, or
 3. an unrefed libuv handle, i.e. does not keep the event loop
    alive, or
 4. an inactive libuv handle (essentially the same here)

There are a few exceptions to this rule, but generally,
if there are C++-backed Node.js objects on the heap
that do not fall into the above categories, we may be looking
at a potential memory leak. Most likely, the cause is a missing
`MakeWeak()` call on the corresponding object.

In order to avoid this kind of problem, we check the list
of BaseObjects for these criteria. In this commit, we only do so
when explicitly instructed to or when in debug mode
(where --verify-base-objects is always-on).

In particular, this avoids the kinds of memory leak issues
that were fixed in the PRs referenced below.

Refs: #35488
Refs: #35487
Refs: #35481

PR-URL: #35490
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@KernelDeimos KernelDeimos mentioned this pull request Sep 9, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@addaleax @nodejs-github-bot @gengjiawen @BethGriggs @mcollina @jasnell @Trott @benjamingr @joyeecheung @targos