GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: do not throw exception after creating FSReqCallback by addaleax · Pull Request #35487 · nodejs/node

Conversation

@addaleax

@addaleax addaleax commented Oct 3, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@addaleax
fs: do not throw exception after creating FSReqCallback
8a58544 
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 Issues and PRs related to file-system APIs and the fs module. label Oct 3, 2020
@addaleax addaleax added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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. Only starts once the PR has an approving review. label Oct 3, 2020
@nodejs-github-bot

nodejs-github-bot commented Oct 3, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33356/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Oct 3, 2020
View reviewed changes
addaleax added a commit to addaleax/node that referenced this pull request Oct 3, 2020
@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 mentioned this pull request Oct 3, 2020
src: add check against non-weak BaseObjects at process exit #35490
Closed
2 tasks done
@nodejs-github-bot

nodejs-github-bot commented Oct 4, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33372/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung approved these changes Oct 4, 2020
View reviewed changes

@joyeecheung joyeecheung left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@addaleax

addaleax commented Oct 4, 2020

Copy link
Copy Markdown
Member Author

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Oct 4, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33382/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Oct 4, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax
fixup! fs: do not throw exception after creating FSReqCallback
4f5c989
@addaleax

addaleax commented Oct 4, 2020

Copy link
Copy Markdown
Member Author

@joyeecheung I’ve added a comment.
joyeecheung reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 5, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 5, 2020
@nodejs-github-bot

nodejs-github-bot commented Oct 5, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33410/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 6, 2020
@github-actions github-actions Bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 6, 2020
@github-actions

github-actions Bot commented Oct 6, 2020

Copy link
Copy Markdown
Contributor
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label Oct 6, 2020
addaleax added a commit that referenced this pull request Oct 6, 2020
@addaleax
fs: do not throw exception after creating FSReqCallback
e817c36 
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 <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@addaleax

addaleax commented Oct 6, 2020

Copy link
Copy Markdown
Member Author

Landed in e817c36
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax closed this Oct 6, 2020
@addaleax
addaleax deleted the fsreqcallback branch October 6, 2020 08:31
@danielleadams danielleadams mentioned this pull request Oct 6, 2020
V14.13.1 proposal #35525
Merged
@baby636 baby636 mentioned this pull request May 10, 2022
[Snyk] Security upgrade gatsby from 2.24.27 to 4.14.0 baby636/node#51
Open
@kissofire kissofire mentioned this pull request May 10, 2022
[Snyk] Security upgrade gatsby from 2.13.73 to 4.14.0 kissofire/node#64
Open
@wedataintelligence wedataintelligence mentioned this pull request May 10, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 4.14.0 wedataintelligence/node#30
Open
@ron-a20 ron-a20 mentioned this pull request May 10, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 4.14.0 ron-a20/node#63
Open
@molllyn1 molllyn1 mentioned this pull request May 11, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 4.14.0 molllyn1/node#45
Open
@RafaelGSS RafaelGSS mentioned this pull request Jul 27, 2022
src,process: initial permission model implementation #44004
Merged
@snyk-bot snyk-bot mentioned this pull request Sep 25, 2022
[Snyk] Fix for 22 vulnerabilities 137717unity/node#70
Open
@wedataintelligence wedataintelligence mentioned this pull request Sep 26, 2022
[Snyk] Fix for 22 vulnerabilities wedataintelligence/node#44
Open
@137717unity 137717unity mentioned this pull request Sep 26, 2022
[Snyk] Fix for 22 vulnerabilities 137717unity/node#73
Open
@snyk-bot snyk-bot mentioned this pull request Sep 27, 2022
[Snyk] Fix for 21 vulnerabilities Rednickle/node-1#56
Open
@wedataintelligence wedataintelligence mentioned this pull request Sep 29, 2022
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#50
Open
This was referenced Sep 29, 2022
[Snyk] Fix for 24 vulnerabilities 137717unity/node#75
Open
[Snyk] Fix for 23 vulnerabilities Rednickle/node-1#60
Open
This was referenced Oct 5, 2022
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#51
Open
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#52
Open
@snyk-bot snyk-bot mentioned this pull request Oct 6, 2022
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#53
Open
@137717unity 137717unity mentioned this pull request Oct 7, 2022
[Snyk] Fix for 24 vulnerabilities 137717unity/node#76
Open
@wedataintelligence wedataintelligence mentioned this pull request Oct 7, 2022
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#54
Open
This was referenced Oct 8, 2022
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#55
Open
[Snyk] Fix for 24 vulnerabilities 137717unity/node#77
Open
@137717unity 137717unity mentioned this pull request Oct 9, 2022
[Snyk] Fix for 24 vulnerabilities 137717unity/node#78
Open
@snyk-bot snyk-bot mentioned this pull request Oct 9, 2022
[Snyk] Fix for 23 vulnerabilities Rednickle/node-1#63
Open
@kissofire kissofire mentioned this pull request Oct 25, 2022
[Snyk] Security upgrade gatsby from 2.13.73 to 4.14.0 kissofire/node#85
Open
@molllyn1 molllyn1 mentioned this pull request Oct 26, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 4.14.0 molllyn1/node#63
Open
@baby636 baby636 mentioned this pull request Oct 26, 2022
[Snyk] Security upgrade gatsby from 2.24.27 to 4.14.0 baby636/node#79
Open
@snyk-bot snyk-bot mentioned this pull request Oct 26, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 4.14.0 ron-a20/node#83
Open
@wedataintelligence wedataintelligence mentioned this pull request Oct 26, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 4.14.0 wedataintelligence/node#61
Open
This was referenced Oct 26, 2022
[Snyk] Security upgrade gatsby from 2.18.18 to 4.14.0 aliceUnhinged613/node#80
Open
[Snyk] Security upgrade gatsby from 2.18.18 to 4.14.0 137717unity/node#83
Open
@joyeecheung joyeecheung mentioned this pull request Jul 9, 2026
fs: preserve readFile() errors for numeric file descriptors #64368
Open
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
@joyeecheung joyeecheung joyeecheung approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

fs Issues and PRs related to file-system APIs and the fs module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@addaleax @nodejs-github-bot @joyeecheung @MylesBorins @jasnell @targos @richardlau
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.