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

crypto: fix KeyObject garbage collection #35481

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 3, 2020

These objects don’t hold any resources on the event loop, so they
should be weak objects that can be garbage collected when nothing
refers to them anymore.

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

These objects don’t hold any resources on the event loop, so they
should be weak objects that can be garbage collected when nothing
refers to them anymore.
@addaleax addaleax requested a review from tniessen October 3, 2020 17:52
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Oct 3, 2020
@addaleax
Copy link
Member Author

addaleax commented Oct 3, 2020

I’d be open to fast-tracking so that #35093 can rebase against this

@addaleax addaleax added request-ci Add this label to start a Jenkins CI on a PR. lts-watch-v12.x labels Oct 3, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 3, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 3, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 3, 2020
@nodejs-github-bot
Copy link
Collaborator

addaleax added a commit that referenced this pull request Oct 3, 2020
These objects don’t hold any resources on the event loop, so they
should be weak objects that can be garbage collected when nothing
refers to them anymore.

PR-URL: #35481
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax
Copy link
Member Author

addaleax commented Oct 3, 2020

Landed in f5c7aa0

@addaleax addaleax closed this Oct 3, 2020
@addaleax addaleax deleted the nko-leak branch October 3, 2020 20:46
addaleax added a commit to addaleax/node that referenced this pull request 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.

Refs: nodejs#35488
Refs: nodejs#35487
Refs: nodejs#35481
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
These objects don’t hold any resources on the event loop, so they
should be weak objects that can be garbage collected when nothing
refers to them anymore.

PR-URL: #35481
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@danielleadams danielleadams mentioned this pull request Oct 6, 2020
addaleax added a commit that referenced this pull request Oct 7, 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.

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>
@addaleax addaleax mentioned this pull request Nov 9, 2020
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
These objects don’t hold any resources on the event loop, so they
should be weak objects that can be garbage collected when nothing
refers to them anymore.

PR-URL: #35481
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
These objects don’t hold any resources on the event loop, so they
should be weak objects that can be garbage collected when nothing
refers to them anymore.

PR-URL: #35481
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
These objects don’t hold any resources on the event loop, so they
should be weak objects that can be garbage collected when nothing
refers to them anymore.

PR-URL: nodejs#35481
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
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
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>
targos pushed a commit that referenced this pull request May 25, 2021
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
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>
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++. crypto Issues and PRs related to the crypto subsystem. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants