fs: filehandle.read(buffer) can't read file when options are omitted

[pulkit-30]

fix: #47183

• make length arg in fh.read(buffer, offset, position, length) default to length ?? buffer.byteLength - offset

Code

import fs from "fs/promises";

async function test() {
  let path = './ex.txt';

  let buf = Buffer.alloc(4);
  let handle;
  try {
    handle = await fs.open(path);
    {
      let buf = Buffer.alloc(4);
      let { bytesRead, buffer } = await handle.read(buf);
      console.log(bytesRead);
      console.log(buffer);
      console.log(buf);
    }
  } finally {
    await handle?.close();
  }
}
test();

before

0
<Buffer 00 00 00 00>
<Buffer 00 00 00 00>

now

4
<Buffer 48 65 6c 6c>
<Buffer 48 65 6c 6c>

---

That's my second attempt at this issue 😅

[nodejs-github-bot]

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

[BridgeAR]

LGTM (while we could remove the length assignment in line 651 and 662)

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/51087
✔  Done loading data for nodejs/node/pull/51087
----------------------------------- PR info ------------------------------------
Title      fs: filehandle.read(buffer) can't read file when options are omitted (#51087)
Author     Pulkit Gupta  (@pulkit-30)
Branch     pulkit-30:fix-47183 -> nodejs:main
Labels     fs, author ready, needs-ci
Commits    3
 - fs: make offset, position & length args in fh.read() optional
 - fix test
 - fix suggestions
Committers 1
 - pulkit-30 
PR-URL: https://github.com/nodejs/node/pull/51087
Fixes: https://github.com/nodejs/node/issues/47183
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Vinícius Lourenço Claro Cardoso 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Luigi Pinca 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51087
Fixes: https://github.com/nodejs/node/issues/47183
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Vinícius Lourenço Claro Cardoso 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Luigi Pinca 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 07 Dec 2023 16:55:28 GMT
   ✔  Approvals: 5
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/51087#pullrequestreview-1774088173
   ✔  - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/51087#pullrequestreview-1777399313
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/51087#pullrequestreview-1777519295
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/51087#pullrequestreview-1780448292
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/51087#pullrequestreview-1793960251
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-12-19T17:04:36Z: https://ci.nodejs.org/job/node-test-pull-request/56391/
- Querying data for job/node-test-pull-request/56391/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 51087
From https://github.com/nodejs/node
 * branch                  refs/pull/51087/merge -> FETCH_HEAD
✔  Fetched commits as 4f3261f10183..1de21b873667
--------------------------------------------------------------------------------
Auto-merging doc/api/fs.md
[main fd0e346463] fs: make offset, position & length args in fh.read() optional
 Author: pulkit-30 
 Date: Thu Dec 7 22:16:46 2023 +0530
 3 files changed, 23 insertions(+), 3 deletions(-)
[main a0f036d617] fix test
 Author: pulkit-30 
 Date: Thu Dec 7 22:17:44 2023 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main dfb1cf7171] fix suggestions
 Author: pulkit-30 
 Date: Fri Dec 8 00:38:07 2023 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fs: make offset, position & length args in fh.read() optional
PR-URL: #51087

Fixes: #47183

Reviewed-By: Benjamin Gruenbaum [email protected]

Reviewed-By: Vinícius Lourenço Claro Cardoso [email protected]

Reviewed-By: Ruben Bridgewater [email protected]

Reviewed-By: Luigi Pinca [email protected]

Reviewed-By: James M Snell [email protected]
[detached HEAD 7932ec3bda] fs: make offset, position & length args in fh.read() optional

Author: pulkit-30 [email protected]

Date: Thu Dec 7 22:16:46 2023 +0530

3 files changed, 23 insertions(+), 3 deletions(-)

Rebasing (3/6)

Rebasing (4/6)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fix test
PR-URL: #51087

Fixes: #47183

Reviewed-By: Benjamin Gruenbaum [email protected]

Reviewed-By: Vinícius Lourenço Claro Cardoso [email protected]

Reviewed-By: Ruben Bridgewater [email protected]

Reviewed-By: Luigi Pinca [email protected]

Reviewed-By: James M Snell [email protected]
[detached HEAD 224a463637] fix test

Author: pulkit-30 [email protected]

Date: Thu Dec 7 22:17:44 2023 +0530

1 file changed, 1 insertion(+), 1 deletion(-)

Rebasing (5/6)

Rebasing (6/6)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fix suggestions
PR-URL: #51087

Fixes: #47183

Reviewed-By: Benjamin Gruenbaum [email protected]

Reviewed-By: Vinícius Lourenço Claro Cardoso [email protected]

Reviewed-By: Ruben Bridgewater [email protected]

Reviewed-By: Luigi Pinca [email protected]

Reviewed-By: James M Snell [email protected]
[detached HEAD c191ac4bbb] fix suggestions

Author: pulkit-30 [email protected]

Date: Fri Dec 8 00:38:07 2023 +0530

1 file changed, 1 insertion(+), 1 deletion(-)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

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

[nodejs-github-bot]

Landed in 48cdb88