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: fix do_not_throw_error argument index in sync fstat by daniellockyer · Pull Request #66637 · nodejs/node

Conversation

@daniellockyer

@daniellockyer daniellockyer commented Oct 10, 2026

Copy link
Copy Markdown

The synchronous branch of the fstat binding read its do_not_throw_error flag from args[2], which is always undefined on that branch, so the flag was ignored and the binding always threw.

Read the flag from args[3] as documented, and restore tryStatSync() to closing the file descriptor it owns before re-throwing the fstat error, which was its behavior before the flag was introduced.

Refs: #49868
Assisted-by: MiniMax M3.1 Flash Preview + Opus 5.5

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Oct 10, 2026
@github-actions

github-actions Bot commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

  • the guide for first-time contributors
  • the contribution and automation policies
  • the pull request guide
  • the AI use policy
  • the Code of Conduct

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daniellockyer
fs: fix do_not_throw_error argument index in sync fstat
593b5bd 
The synchronous branch of the fstat binding read its
do_not_throw_error flag from args[2], which is always undefined on
that branch, so the flag was ignored and the binding always threw.

Read the flag from args[3] as documented, and restore tryStatSync()
to closing the file descriptor it owns before rethrowing the fstat
error, which was its behaviour before the flag was introduced.

Refs: nodejs#49868
Assisted-by: MiniMax M3.1 Flash Preview + Opus 5.5
Signed-off-by: Daniel Lockyer <[email protected]>
@daniellockyer
daniellockyer force-pushed the fs-fstat-sync-arg-index branch from 2ea782d to 593b5bd Compare October 10, 2026 07:09
addaleax
addaleax reviewed Oct 30, 2026
View reviewed changes
Comment thread lib/fs.js
stats = binding.fstat(fd, false, undefined, false);
threw = false;
} finally {
if (threw && !isUserFd) fs.closeSync(fd);
}

@addaleax addaleax Oct 30, 2026

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 you explain why we're switching shouldNotThrow from true to false and adding try/catch? The C++ fix seems unambiguously correct, but the motivation for the corresponding JS changes isn't really explained anywhere

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax left review comments

At least 1 approving review is required to merge this pull request.

Uh oh!

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

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to file-system APIs and the fs module. 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.

Uh oh!

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

3 participants

@daniellockyer @addaleax @nodejs-github-bot
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.