Skip to content

fs: adjust truncate mode from r+ to a#43315

Open
LiviaMedeiros wants to merge 1 commit intonodejs:mainfrom
LiviaMedeiros:fs-truncate-mode-append
Open

fs: adjust truncate mode from r+ to a#43315
LiviaMedeiros wants to merge 1 commit intonodejs:mainfrom
LiviaMedeiros:fs-truncate-mode-append

Conversation

@LiviaMedeiros
Copy link
Member

@LiviaMedeiros LiviaMedeiros commented Jun 5, 2022

This PR changes flags in fsPromises.truncate(path[, len]), fs.truncate(path[, len], callback), and fs.truncateSync(path[, len]) from r+ to a.

This change:

  • Allows to truncate files without read permission (previously: EACCES)
  • Allows to create a zero-filled file of specified length if it didn't exist at specified path

Creating file is not what pure truncate(2) does, but it is done by truncate(1) and kinda makes sense.

Alternatively, we can check for W_OK and throw ENOENT manually.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jun 5, 2022
@LiviaMedeiros LiviaMedeiros added semver-major PRs that contain breaking changes and should be released in the next major version. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. and removed fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jun 5, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jun 5, 2022

/cc @nodejs/fs

RaisinTen
RaisinTen approved these changes Jun 12, 2022
View reviewed changes
Copy link
Member

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM. Should we change r+ -> a in the docs too?

node/doc/api/fs.md

Line 586 in 71071f8

filehandle = await open('temp.txt', 'r+');

node/doc/api/fs.md

Line 2747 in 71071f8

open('temp.txt', 'r+', (err, fd) => {

@LiviaMedeiros
Copy link
Member Author

LiviaMedeiros commented Jun 12, 2022

Should we change r+ -> a in the docs too?

I don't think so. These examples are for filehandle.truncate(len) and fs.ftruncate(fd[, len], callback). Both methods are unaffected by this change, and both methods should be used in more complex scenarios. r+ effectively implies that filehandle or fd have untold adventures.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 24, 2022

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

This was referenced Jun 25, 2022
Open
Open
Open
Open
@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 8, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 8, 2022

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

This was referenced Sep 9, 2022
Open
Open
Open
@LiviaMedeiros LiviaMedeiros force-pushed the fs-truncate-mode-append branch from b544846 to 375f0a7 Compare September 11, 2022 05:37
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 11, 2022

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

aduh95
aduh95 previously approved these changes Sep 11, 2022
View reviewed changes
@aduh95 aduh95 dismissed their stale review September 11, 2022 10:35

There are a bunch of seemingly related CI failures on Windows.

@LiviaMedeiros LiviaMedeiros removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 11, 2022
@LiviaMedeiros
Copy link
Member Author

LiviaMedeiros commented Sep 11, 2022 

node:internal/fs/utils:348
    throw err;
    ^

Error: EPERM: operation not permitted, ftruncate
    at Object.ftruncateSync (node:fs:1136:3)
    at Object.truncateSync (node:fs:1095:14)
    at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-fs-truncate.js:44:4)
    at Module._compile (node:internal/modules/cjs/loader:1129:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1183:10)
    at Module.load (node:internal/modules/cjs/loader:1007:32)
    at Module._load (node:internal/modules/cjs/loader:848:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:86:12)
    at node:internal/main/run_main_module:23:47 {
  errno: -4048,
  syscall: 'ftruncate',
  code: 'EPERM'
}

Node.js v19.0.0-pre

On a first glance, it looks like a bug in underlying layer. Most likely an attempt to allocate a sparse zero-filled range on a filesystem (probably FAT?) that doesn't support that.

This was referenced Sep 12, 2022
Open
Open
Open
Open
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 10, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 10, 2022

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

@github-actions github-actions bot mentioned this pull request Oct 11, 2022
Open
15 tasks
@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 11, 2022
@aduh95
Copy link
Contributor

aduh95 commented Oct 11, 2022

Unfortunately, there are still failures on Windows, this will probably need to wait Node.js 20.0.0 if we are able to fix/workaround the failures.

@github-actions github-actions bot mentioned this pull request Oct 12, 2022
Open
19 tasks
@aduh95
Copy link
Contributor

aduh95 commented Feb 22, 2025

This PR has conflicts that must be resolved

@LiviaMedeiros LiviaMedeiros force-pushed the fs-truncate-mode-append branch from 375f0a7 to 66f94ce Compare February 28, 2025 06:30
@LiviaMedeiros LiviaMedeiros added wip Issues and PRs that are still a work in progress. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 28, 2025
@LiviaMedeiros
Copy link
Member Author

LiviaMedeiros commented Feb 28, 2025

Rebased, let's see if it still causes issues on Windows.
wip Issues and PRs that are still a work in progress. because the test has to be updated (assuming we can have this change)

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 28, 2025

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

@codecov
Copy link

codecov bot commented Feb 28, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.26%. Comparing base (4ec9d42) to head (66f94ce).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #43315      +/-   ##
==========================================
- Coverage   90.26%   90.26%   -0.01%     
==========================================
  Files         630      630              
  Lines      184690   184690              
  Branches    36157    36140      -17     
==========================================
- Hits       166718   166714       -4     
  Misses      11021    11021              
- Partials     6951     6955       +4
Files with missing lines Coverage Δ
lib/fs.js 98.25% <100.00%> (-0.03%) ⬇️
lib/internal/fs/promises.js 98.16% <100.00%> (-0.08%) ⬇️

... and 34 files with indirect coverage changes

@aduh95
Copy link
Contributor

aduh95 commented Feb 28, 2025

Getting Error: EPERM: operation not permitted, ftruncate on Windows CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@aduh95 aduh95 aduh95 left review comments

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

Assignees

No one assigned

Labels

fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. wip Issues and PRs that are still a work in progress.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@LiviaMedeiros @aduh95 @nodejs-github-bot @bnoordhuis @jasnell @lpinca @RaisinTen