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: adjust truncate mode from r+ to a #43315

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LiviaMedeiros
Copy link
Contributor

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

Copy link
Contributor

@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?

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

@LiviaMedeiros
Copy link
Contributor Author

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

@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

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

@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

aduh95
aduh95 previously approved these changes Sep 11, 2022
@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
Contributor Author

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.

@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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants