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.

Sign up for GitHub

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

Jump to bottom

[v14.x] backport process.exit changes #38786

Closed
wants to merge 3 commits into from
Closed

[v14.x] backport process.exit changes #38786

wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

addaleax added 3 commits May 23, 2021 16:37
@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
src: fix compiler warning in env.cc
09cfd99 
     ../src/env.cc:1227:22: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
      ForEachBaseObject([this](BaseObject* obj) {
                     ^~~~

PR-URL: nodejs#35547
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@addaleax
src: add maybe versions of EmitExit and EmitBeforeExit
efcc595 
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: nodejs#35486
Reviewed-By: James M Snell <jasnell@gmail.com>
@github-actions github-actions 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. needs-ci PRs that need a full CI run. v14.x labels May 23, 2021
@nodejs-github-bot
Copy link
Collaborator

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

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 pushed a commit that referenced this pull request May 25, 2021
@addaleax @targos
src: fix compiler warning in env.cc
3e05f76 
     ../src/env.cc:1227:22: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
      ForEachBaseObject([this](BaseObject* obj) {
                     ^~~~

PR-URL: #35547
Backport-PR-URL: #38786
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos pushed a commit that referenced this pull request May 25, 2021
@addaleax @targos
src: add maybe versions of EmitExit and EmitBeforeExit
2ec28e4 
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos
Copy link
Member

targos commented May 25, 2021

Landed in 55d0cd8...2ec28e4

@targos targos closed this May 25, 2021
targos pushed a commit that referenced this pull request Jun 6, 2021
@addaleax @targos
src: add maybe versions of EmitExit and EmitBeforeExit
6c2f31a 
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jun 6, 2021
@addaleax @targos
src: add maybe versions of EmitExit and EmitBeforeExit
49c8441 
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
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>
targos pushed a commit that referenced this pull request Jun 11, 2021
@addaleax @targos
src: fix compiler warning in env.cc
78cde14 
     ../src/env.cc:1227:22: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
      ForEachBaseObject([this](BaseObject* obj) {
                     ^~~~

PR-URL: #35547
Backport-PR-URL: #38786
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
@addaleax @targos
src: add maybe versions of EmitExit and EmitBeforeExit
50b3bd0 
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jun 15, 2021
@addaleax @targos
src: add maybe versions of EmitExit and EmitBeforeExit
f04a702 
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 5, 2021
@addaleax @richardlau
src: add maybe versions of EmitExit and EmitBeforeExit
16dcd9c 
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 29, 2021
@addaleax @richardlau
src: add maybe versions of EmitExit and EmitBeforeExit
0061e5d 
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 12, 2021
@addaleax @BethGriggs
src: add maybe versions of EmitExit and EmitBeforeExit
310f6c9 
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 31, 2021
@addaleax @MylesBorins
src: add maybe versions of EmitExit and EmitBeforeExit
f421422 
This addresses a TODO comment, and removes invalid `.ToLocalChecked()`
calls from our code base.

PR-URL: #35486
Backport-PR-URL: #38786
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@addaleax @nodejs-github-bot @targos @jasnell