The Wayback Machine - https://web.archive.org/web/20220423193232/https://github.com/nodejs/node/pull/35487
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

fs: do not throw exception after creating FSReqCallback #35487

Closed
wants to merge 2 commits into from

Conversation

Copy link
Member

@addaleax addaleax commented Oct 3, 2020

Once an FSReqCallback instance is created, it is a GC root until
the underlying fs operation has completed, meaning that it cannot
be garbage collected.

This is a problem when the underlying operation never starts
because an exception is thrown before that happens, for example
as part of parameter validation.

Instead, move all potentially throwing code before the FSReqCallback
creation.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Once an `FSReqCallback` instance is created, it is a GC root until
the underlying fs operation has completed, meaning that it cannot
be garbage collected.

This is a problem when the underlying operation never starts
because an exception is thrown before that happens, for example
as part of parameter validation.

Instead, move all potentially throwing code before the `FSReqCallback`
creation.
@nodejs-github-bot nodejs-github-bot added the fs label Oct 3, 2020
@addaleax addaleax added request-ci lts-watch-v12.x labels Oct 3, 2020
@github-actions github-actions bot removed the request-ci label Oct 3, 2020
addaleax added a commit to addaleax/node that referenced this issue 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
Copy link
Member

@joyeecheung joyeecheung left a comment

Can we document this somehow? Otherwise next time someone may add a new method that does this again and we would have to catch it through code reviews

@addaleax
Copy link
Member Author

@addaleax addaleax commented Oct 4, 2020

@joyeecheung I don’t really know where to document this, but #35490 would make CI fail if this happens again, so I think we’re okay there

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 4, 2020

@addaleax Maybe at the top of the file? (I think when someone tries to add a new method they would probably look there). I imagine when the CI fails they would just see the callback being printed before exit which could be difficult for new comers to understand..

@addaleax
Copy link
Member Author

@addaleax addaleax commented Oct 4, 2020

@joyeecheung I’ve added a comment.

@addaleax addaleax added the request-ci label Oct 5, 2020
@github-actions github-actions bot removed the request-ci label Oct 5, 2020
@addaleax addaleax added the commit-queue label Oct 6, 2020
@github-actions github-actions bot removed the commit-queue label Oct 6, 2020
@github-actions
Copy link

@github-actions github-actions bot commented Oct 6, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/35487
✔  Done loading data for nodejs/node/pull/35487
----------------------------------- PR info ------------------------------------
Title      fs: do not throw exception after creating FSReqCallback (#35487)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     addaleax:fsreqcallback -> nodejs:master
Labels     fs, lts-watch-v12.x
Commits    2
 - fs: do not throw exception after creating FSReqCallback
 - fixup! fs: do not throw exception after creating FSReqCallback
Committers 1
 - Anna Henningsen 
PR-URL: https://github.com/nodejs/node/pull/35487
Reviewed-By: James M Snell 
Reviewed-By: Joyee Cheung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35487
Reviewed-By: James M Snell 
Reviewed-By: Joyee Cheung 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup! fs: do not throw exception after creating FSReqCallback
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-10-05T20:23:13Z: https://ci.nodejs.org/job/node-test-pull-request/33410/
- Querying data for job/node-test-pull-request/33410/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sat, 03 Oct 2020 22:43:13 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/35487#pullrequestreview-501578948
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/35487#pullrequestreview-501630754
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
@github-actions github-actions bot added the commit-queue-failed label Oct 6, 2020
addaleax added a commit that referenced this issue Oct 6, 2020
Once an `FSReqCallback` instance is created, it is a GC root until
the underlying fs operation has completed, meaning that it cannot
be garbage collected.

This is a problem when the underlying operation never starts
because an exception is thrown before that happens, for example
as part of parameter validation.

Instead, move all potentially throwing code before the `FSReqCallback`
creation.

PR-URL: #35487
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@addaleax
Copy link
Member Author

@addaleax addaleax commented Oct 6, 2020

Landed in e817c36

@addaleax addaleax closed this Oct 6, 2020
@addaleax addaleax deleted the fsreqcallback branch Oct 6, 2020
@danielleadams danielleadams mentioned this pull request Oct 6, 2020
danielleadams pushed a commit that referenced this issue Oct 6, 2020
Once an `FSReqCallback` instance is created, it is a GC root until
the underlying fs operation has completed, meaning that it cannot
be garbage collected.

This is a problem when the underlying operation never starts
because an exception is thrown before that happens, for example
as part of parameter validation.

Instead, move all potentially throwing code before the `FSReqCallback`
creation.

PR-URL: #35487
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit that referenced this issue 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>
@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Nov 16, 2020

This doesn't land cleanly on 12.x. As 12.x is about to move to maintenance I'm unsure if it makes sense to backport. We could still potentially get this in the 12.20.0 release if it is backported in a timely fashion, but it isn't clear that this is high priority.

joesepi pushed a commit to joesepi/node that referenced this issue Jan 8, 2021
Once an `FSReqCallback` instance is created, it is a GC root until
the underlying fs operation has completed, meaning that it cannot
be garbage collected.

This is a problem when the underlying operation never starts
because an exception is thrown before that happens, for example
as part of parameter validation.

Instead, move all potentially throwing code before the `FSReqCallback`
creation.

PR-URL: nodejs#35487
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this issue 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 issue 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 issue 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 issue 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>
@targos targos removed the commit-queue-failed label Sep 5, 2021
@richardlau richardlau added the backport-requested-v12.x label Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v12.x fs lts-watch-v12.x
7 participants