fs: add read(buffer[, options]) versions #42768

LiviaMedeiros · 2022-04-17T17:15:02Z

Refs: #42601 (writing counterparts)

This PR does:

Adding fs.read(fd, buffer[, options], callback)
Adding filehandle.read(buffer[, options])
~~Aligning fs.readSync code (changes in UB corner cases) ¹~~ (offloaded due to potential breakage)

These changes would allow to:

Align writing methods with uniform buffer[, options] signature ²
Align signatures of reading methods
Which also means not only read-write interoperability within each of three APIs, but also between any of them
Achieve a bit more consistent UB behaviour, when documentation implies similarity ("For detailed information, see the documentation of the asynchronous version of this API")
Have it fully completed without waiting for DEP0162 EOL
Harden fs.readSync typechecking ³
(optional) Introduce params⁴ signatures later, if needed ⁵
(optional) Deprecate and remove params signatures later, if strongly needed ⁶
(opinionated) Maybe make usage of fs.read and fh.read less error-prone ⁶

Why current behaviour is questionable

Default buffer in fs.read and filehandle.read is Buffer.alloc(16384). It's a pretty reasonable value for positioned argument (i.e. if it's used only when all further arguments are unset as well), but it's a footgun when length is defined.

#!/usr/bin/env node
import {open} from 'fs/promises';
async function readSubstring(filepath, len = 0, pos = 0) {
	const fh = await open(filepath, 'r');
	const res = await fh.read({ length: Number(len), position: Number(pos) });
	const str = String(res.buffer);
	return str;
}
const substr = await readSubstring(...process.argv.slice(2));
console.log(substr);

$ echo 123456789 > /tmp/tst
$ readsubstr.mjs /tmp/tst 3 5
678
$ readsubstr.mjs /tmp/tst 3 5 | wc -c
16385

Without reading the docs properly, I'd totes assume that it allocates length + offset||0 number of bytes by default.
Because of that, buffer has to be allocated explicitly. Which makes [buffer[, options]] signature more reasonable and less misleading.

Changes in UB are about interpreting false/null/undefined values as invalid or default. I think, the best way here is to interpret typeof offsetOrOptions === 'object' as indicator of attempt to use "named params" version. null means default values. false and undefined means offset === 0. Everything else goes to offset's integer validation.
Exception: Callback API methods will still check the number of arguments as well, to keep callback strictly positioned. ↩
Refs: #42601 (comment) ↩
Current implementation of fs.readSync interprets any value as options object. For example, passing a primitive string will use its length property without any warning. ↩
I use the following terminology here: options == { offset, length, position }, params == { buffer, ...options } == { buffer, offset, length, position } ↩
Theoretical reason to include buffer is its strong connection to offset and length. If we specify nonzero offset, we usually imply a non-arbitrary buffer. However, I don't have strong arguments nor good examples; and it can be worked around anyway. ↩
See "Why current behaviour is questionable" foldablie ↩ ↩²

This adds the following: `fs.read(fd, buffer[, options], callback)` `filehandle.read(buffer[, options])`

test/parallel/test-fs-read-offset-null.js

LiviaMedeiros · 2022-04-19T08:50:27Z

Offloaded fs.readSync changes to a separate PR.

I'm still confused about offset === null and options.offset === null case.

In #32479, setting any option to null was prohibited, but the test was bugged.
As for today, position == null || position === -1 has a special meaning ("read at current position and update it"). length is silently coerced to integer. offset is either coerced to 0 or interpreted as default options (in readSync version).
Later, #35918 introduced opposite (passing) test for { offset: null }.
This test is changed in this PR, original version won't pass.
In #38517, in particular #38517 (comment), the original test was removed as de-facto not working.

I believe that the intention behind original test was purely about default params (so, for example, it won't override position it default value for it was 0).
Is there something else, and should or not offset === null throw?

My personal preference in a context of this PR is to prohibit it to avoid any intersection with options object, purely because typeof null === 'object'.
However, if supporting nullable offset is the desired behaviour, it can be worked around to avoid any breaking changes here.

LiviaMedeiros · 2022-04-22T12:46:09Z

In current state or this PR, breaking changes for fs.readSync are reverted, and handling of offset === null cases should be compatible with master branch. I'm doing brief rechecks with this.

If there are still any signs of breaking changes, please do not hesitate to point them out.
I believe that it can be kept within semver-minor without functionality loss.

test/parallel/test-fs-read-optional-params.js

test/parallel/test-fs-read-offset-null.js

nodejs-github-bot · 2022-04-22T18:45:38Z

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

nodejs-github-bot · 2022-04-29T02:55:25Z

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

nodejs-github-bot · 2022-05-02T17:49:34Z

Commit Queue failed

- Loading data for nodejs/node/pull/42768
✔  Done loading data for nodejs/node/pull/42768
----------------------------------- PR info ------------------------------------
Title      fs: add read(buffer[, options]) versions (#42768)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     LiviaMedeiros:optional-read-signature -> nodejs:master
Labels     fs, semver-minor, author ready, needs-ci, commit-queue-squash
Commits    16
 - fs: add read(buffer, options) signatures
 - squash: make offsetOrOptions more consistent
 - squash: add doc entries
 - squash: fix fs.read()
 - squash: expand fs-read-optional test
 - squash: expand fs-read-promises-optional test
 - squash: improve tests
 - squash: fixup
 - squash: fixup
 - squash: inline null coalescing
 - squash: revert changes in fs.readSync()
 - squash: disallow offset === false
 - squash: fixup
 - squash: allow offset === null
 - squash: fix tests being other way round
 - squash: adjust tests
Committers 1
 - LiviaMedeiros 
PR-URL: https://github.com/nodejs/node/pull/42768
Refs: https://github.com/nodejs/node/pull/42601
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42768
Refs: https://github.com/nodejs/node/pull/42601
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 17 Apr 2022 17:15:02 GMT
   ✔  Approvals: 1
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/42768#pullrequestreview-950452626
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-04-29T02:55:25Z: https://ci.nodejs.org/job/node-test-pull-request/43764/
- Querying data for job/node-test-pull-request/43764/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 42768
From https://github.com/nodejs/node
 * branch                  refs/pull/42768/merge -> FETCH_HEAD
✔  Fetched commits as 916a13a8a36d..ab0b534874af
--------------------------------------------------------------------------------
Auto-merging lib/fs.js
[master 4b272000d4] fs: add read(buffer, options) signatures
 Author: LiviaMedeiros 
 Date: Fri Apr 15 22:01:15 2022 +0800
 3 files changed, 37 insertions(+), 30 deletions(-)
Auto-merging lib/fs.js
CONFLICT (content): Merge conflict in lib/fs.js
error: could not apply aec64cae9d... squash: make offsetOrOptions more consistent
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
   ✖  Failed to apply patches

https://github.com/nodejs/node/actions/runs/2259461302

LiviaMedeiros · 2022-05-02T17:59:03Z

Rebasing this atm

aduh95 · 2022-05-02T18:01:37Z

Landed in 57678e5

LiviaMedeiros · 2022-05-02T18:07:09Z

Oh, thanks! 😄

This adds the following: - `fs.read(fd, buffer[, options], callback)`. - `filehandle.read(buffer[, options])`. PR-URL: #42768 Refs: #42601 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Notable changes: * Revert_ "deps: add template for generated headers" (Daniel Bevenius) #42978 * deps: upgrade npm to 8.9.0 (npm team) #42968 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (Livia Medeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 PR-URL: TBD

Notable changes: * Revert_ "deps: add template for generated headers" (Daniel Bevenius) #42978 * deps: upgrade npm to 8.9.0 (npm team) #42968 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (Livia Medeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 PR-URL: #43036

Notable changes: * This release updates OpenSSL to 3.0.3. This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18. See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines. * *Revert*_ "deps: add template for generated headers" (Daniel Bevenius) #42978 * deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022 * deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059 * deps: upgrade npm to 8.9.0 (npm team) #42968 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 * deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022 * doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039 * doc: add release key for Juan Arboleda (Juan José) #42961 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812 * perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725 PR-URL: #43036

OpenSSL This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18. See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines. * deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022 * deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022 Other Notable changes * *Revert*_ "deps: add template for generated headers" (Daniel Bevenius) #42978 * deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059 * deps: upgrade npm to 8.9.0 (npm team) #42968 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 * doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039 * doc: add release key for Juan Arboleda (Juan José) #42961 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812 * perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725 PR-URL: #43036

OpenSSL This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18. See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines. * deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022 * deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022 Other Notable Changes * *Revert*_ "deps: add template for generated headers" (Daniel Bevenius) #42978 * deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059 * deps: upgrade npm to 8.9.0 (npm team) #42968 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 * doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039 * doc: add release key for Juan Arboleda (Juan José) #42961 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812 * perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725 PR-URL: #43036

Notable changes: OpenSSL 3.0.3 This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18. See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines. - deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022 - deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022 Other notable changes: - _Revert_ "deps: add template for generated headers" (Daniel Bevenius) #42978 - deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059 - deps: upgrade npm to 8.9.0 (npm team) #42968 - (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 - (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 - doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039 - doc: add release key for Juan Arboleda (Juan José) #42961 - (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 - (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812 - (SEMVER-MINOR) perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725 PR-URL: #43036

LiviaMedeiros added 6 commits Apr 17, 2022

fs: add read(buffer, options) signatures

c69abaa

This adds the following: `fs.read(fd, buffer[, options], callback)` `filehandle.read(buffer[, options])`

squash: make offsetOrOptions more consistent

aec64ca

squash: add doc entries

42c9c40

squash: fix fs.read()

71f03ce

squash: expand fs-read-optional test

0bd9de7

squash: expand fs-read-promises-optional test

abd6e4f

nodejs-github-bot added fs needs-ci labels Apr 17, 2022

LiviaMedeiros mentioned this pull request Apr 17, 2022

fs: make params in writing methods optional #42601

Merged

aduh95 reviewed Apr 17, 2022

View changes

test/parallel/test-fs-read-offset-null.js Outdated Show resolved Hide resolved

aduh95 added the semver-major label Apr 17, 2022

LiviaMedeiros added 5 commits Apr 18, 2022

squash: improve tests

9826309

squash: fixup

a32c914

squash: fixup

6a974c2

squash: inline null coalescing

0f586d2

squash: revert changes in fs.readSync()

6410dec

squash: disallow offset === false

ccaa4cb

LiviaMedeiros mentioned this pull request Apr 19, 2022

fs: harden options typecheck in reading methods #42772

Open

LiviaMedeiros added 3 commits Apr 21, 2022

squash: fixup

0e6b06a

squash: allow offset === null

42b640c

squash: fix tests being other way round

e153143

aduh95 reviewed Apr 22, 2022

View changes

test/parallel/test-fs-read-optional-params.js Outdated Show resolved Hide resolved

test/parallel/test-fs-read-offset-null.js Outdated Show resolved Hide resolved

test/parallel/test-fs-read-offset-null.js Show resolved Hide resolved

squash: adjust tests

ab0b534

aduh95 approved these changes Apr 22, 2022

View changes

aduh95 added semver-minor author ready request-ci and removed semver-major labels Apr 22, 2022

github-actions bot removed the request-ci label Apr 22, 2022

github-actions bot mentioned this pull request Apr 23, 2022

CI Reliability 2022-04-23 nodejs/reliability#260

Open

21 tasks

github-actions bot mentioned this pull request Apr 24, 2022

CI Reliability 2022-04-24 nodejs/reliability#261

Open

21 tasks

github-actions bot mentioned this pull request Apr 25, 2022

CI Reliability 2022-04-25 nodejs/reliability#262

Open

16 tasks

github-actions bot mentioned this pull request Apr 26, 2022

CI Reliability 2022-04-26 nodejs/reliability#263

Open

14 tasks

github-actions bot mentioned this pull request Apr 27, 2022

CI Reliability 2022-04-27 nodejs/reliability#264

Open

11 tasks

github-actions bot mentioned this pull request Apr 28, 2022

CI Reliability 2022-04-28 nodejs/reliability#265

Open

18 tasks

aduh95 added commit-queue commit-queue-squash labels May 2, 2022

nodejs-github-bot added commit-queue-failed and removed commit-queue labels May 2, 2022

aduh95 merged commit 57678e5 into nodejs:master May 2, 2022
59 checks passed

RafaelGSS mentioned this pull request May 10, 2022

v18.2.0 proposal #43036

Merged

nodejs / node Public

fs: add read(buffer[, options]) versions #42768

fs: add read(buffer[, options]) versions #42768

LiviaMedeiros commented Apr 17, 2022 •

edited

LiviaMedeiros commented Apr 19, 2022

LiviaMedeiros commented Apr 22, 2022

nodejs-github-bot commented Apr 22, 2022

nodejs-github-bot commented Apr 29, 2022

nodejs-github-bot commented May 2, 2022

LiviaMedeiros commented May 2, 2022

aduh95 commented May 2, 2022

LiviaMedeiros commented May 2, 2022

nodejs / node Public

fs: add read(buffer[, options]) versions #42768

fs: add read(buffer[, options]) versions #42768

Conversation

LiviaMedeiros commented Apr 17, 2022 • edited

Footnotes

LiviaMedeiros commented Apr 19, 2022

LiviaMedeiros commented Apr 22, 2022

nodejs-github-bot commented Apr 22, 2022

nodejs-github-bot commented Apr 29, 2022

nodejs-github-bot commented May 2, 2022

LiviaMedeiros commented May 2, 2022

aduh95 commented May 2, 2022

LiviaMedeiros commented May 2, 2022

LiviaMedeiros commented Apr 17, 2022 •

edited