Skip to content

fs: make sure to write entire buffer#49211

Closed
atlowChemi wants to merge 0 commit into
nodejs:mainfrom
atlowChemi:write-all
Closed

fs: make sure to write entire buffer#49211
atlowChemi wants to merge 0 commit into
nodejs:mainfrom
atlowChemi:write-all

Conversation

@atlowChemi
Copy link
Copy Markdown
Member

@atlowChemi atlowChemi commented Aug 17, 2023

fs.write(v) is not guaranteed to write everything in a single
call. Make sure we don't assume so.

Refs: #42434 (comment)

@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 Aug 17, 2023
anonrig
anonrig reviewed Aug 17, 2023
View reviewed changes
Comment thread lib/internal/fs/streams.js
Comment thread lib/internal/fs/streams.js
@mscdex mscdex added the needs-benchmark-ci PR that need a benchmark CI run. label Aug 17, 2023
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2023
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 20, 2023

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

@atlowChemi
Copy link
Copy Markdown
Member Author

atlowChemi commented Aug 21, 2023

benchmark output:

                                                                                              confidence improvement accuracy (*)    (**)   (***)
fs/write-stream-throughput.js size=1024 encodingType='asc' dur=5                                             -0.97 %       ±2.40%  ±3.19%  ±4.15%
fs/write-stream-throughput.js size=1024 encodingType='buf' dur=5                                              0.81 %       ±1.53%  ±2.03%  ±2.65%
fs/write-stream-throughput.js size=1024 encodingType='utf' dur=5                                              1.61 %       ±1.75%  ±2.33%  ±3.06%
fs/write-stream-throughput.js size=1048576 encodingType='asc' dur=5                                           0.09 %       ±2.32%  ±3.09%  ±4.02%
fs/write-stream-throughput.js size=1048576 encodingType='buf' dur=5                                          -0.38 %       ±0.60%  ±0.80%  ±1.03%
fs/write-stream-throughput.js size=1048576 encodingType='utf' dur=5                                          -0.38 %       ±1.50%  ±1.99%  ±2.60%
fs/write-stream-throughput.js size=2 encodingType='asc' dur=5                                                -0.31 %       ±2.03%  ±2.71%  ±3.53%
fs/write-stream-throughput.js size=2 encodingType='buf' dur=5                                                -0.53 %       ±1.74%  ±2.32%  ±3.02%
fs/write-stream-throughput.js size=2 encodingType='utf' dur=5                                                -0.13 %       ±2.22%  ±2.96%  ±3.85%
fs/write-stream-throughput.js size=65535 encodingType='asc' dur=5                                     **     -1.64 %       ±1.05%  ±1.40%  ±1.85%
fs/write-stream-throughput.js size=65535 encodingType='buf' dur=5                                             0.09 %       ±1.02%  ±1.37%  ±1.80%
fs/write-stream-throughput.js size=65535 encodingType='utf' dur=5                                             0.06 %       ±1.18%  ±1.57%  ±2.04%


Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 142 comparisons, you can thus
expect the following amount of false-positive results:
  7.10 false positives, when considering a   5% risk acceptance (*, **, ***),
  1.42 false positives, when considering a   1% risk acceptance (**, ***),
  0.14 false positives, when considering a 0.1% risk acceptance (***)

@atlowChemi atlowChemi requested a review from anonrig August 21, 2023 07:30
ronag
ronag reviewed Aug 21, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Existing tests pass and it's quite tricky make tests for this. You probably need to pass a custom "fs" implementation into the stream which will return EAGAIN.

ronag
ronag reviewed Aug 22, 2023
View reviewed changes
Comment thread test/parallel/test-fs-write-stream-eagain.mjs Outdated
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2023
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 22, 2023

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

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2023
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 22, 2023

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

@github-actions github-actions Bot mentioned this pull request Aug 23, 2023
Open
27 tasks
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 23, 2023

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 23, 2023

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

ronag
ronag reviewed Aug 23, 2023
View reviewed changes
Comment thread lib/internal/fs/streams.js Outdated
@atlowChemi atlowChemi force-pushed the write-all branch 2 times, most recently from 2c9b7e0 to 20d51d2 Compare August 23, 2023 13:44
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2023
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 23, 2023

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 23, 2023

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 23, 2023

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

@github-actions github-actions Bot mentioned this pull request Aug 24, 2023
Open
27 tasks
@atlowChemi atlowChemi added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 24, 2023
atlowChemi added a commit to atlowChemi/node that referenced this pull request Aug 24, 2023
@ronag @atlowChemi
fs: make sure to write entire buffer
feb5b0f 
fs.write(v) is not guaranteed to write everything in a single
call. Make sure we don't assume so.

PR-URL: nodejs#49211
Co-authored-by: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@atlowChemi
Copy link
Copy Markdown
Member Author

atlowChemi commented Aug 24, 2023

Landed in feb5b0f

@atlowChemi atlowChemi closed this Aug 24, 2023
@atlowChemi atlowChemi deleted the write-all branch August 24, 2023 05:36
This was referenced Aug 25, 2023
Open
Open
Open
Open
Open
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@ronag @atlowChemi @UlisesGascon
fs: make sure to write entire buffer
88ba79b 
fs.write(v) is not guaranteed to write everything in a single
call. Make sure we don't assume so.

PR-URL: #49211
Co-authored-by: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
Merged
@aduh95 aduh95 mentioned this pull request Sep 20, 2023
Closed
richardlau pushed a commit that referenced this pull request Sep 28, 2023
@ronag @atlowChemi @richardlau
fs: make sure to write entire buffer
7a3e1ff 
fs.write(v) is not guaranteed to write everything in a single
call. Make sure we don't assume so.

PR-URL: #49211
Co-authored-by: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
This was referenced Oct 6, 2023
Closed
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@ronag @atlowChemi @alexfernandez
fs: make sure to write entire buffer
febb608 
fs.write(v) is not guaranteed to write everything in a single
call. Make sure we don't assume so.

PR-URL: nodejs#49211
Co-authored-by: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
kriscendobot pushed a commit to endojs/endo-but-for-bots that referenced this pull request May 18, 2026
ci: preserve Node 20 SES-viable patch history (per kriskowal review on …
c040dc7 
…#280)

The 2ec645b commit ("chore(ci): drop Node.js 20 from the test matrix")
dropped the `test-async-hooks` matrix's `'20'` entry alongside the
four-line comment block that documented which Node-20 patch versions
were SES-viable. Per kriskowal's CHANGES_REQUESTED review on #280
("Please preserve the historical record on version compatibility."),
restore that commentary above the surviving `'22'` entry, framed as
a historical record so future readers see the lineage rather than
floating annotations.

The restored block names:
  - '20.3' to '20.6' not viable due to nodejs/node#49211
  - '20.6' not viable due to nodejs/node#49497
  - '20.7' first SES-viable version of Node 20
  - '20.9' first LTS of Node 20

and explicitly identifies `'22'` as the post-promise-fast-path lane on
a current LTS (the SES-viable promise-hook lane the Node-20 entry was
guarding), wording lifted from 2ec645b's commit body so the workflow
file carries the rationale in-line.
@kriscendobot kriscendobot mentioned this pull request May 18, 2026
Open
kriskowal added a commit to endojs/endo that referenced this pull request May 18, 2026
@kriskowal
chore(ci): drop Node.js 20 from the test matrix
6d007fa 
Node 20 enters maintenance and is winding down through its LTS
lifecycle. Removing it from the CI matrix narrows the matrix to 18
(still on maintenance), 22 (active LTS), and 24 (current).

The test, cover, test262, and viable-release matrices drop their '20'
entries. Standalone Node-20 jobs (test-xs, test-ocapn-python, and the
ocapn-guile-interop workflow) advance to 22.x rather than being
removed. The test-async-hooks matrix's '20' entry advances to '22'
(the post-promise-fast-path lane on a current LTS, the SES-viable
promise-hook lane the Node-20 entry was guarding). The `test-xs
(macos-15)` lane on Node 20 was filed as flaky.

A historical record of which Node-20 patch versions were SES-viable
is preserved as commentary above the surviving '22' entry in the
test-async-hooks matrix, so future readers see the lineage rather
than floating annotations:

  - '20.3' to '20.6' not viable due to nodejs/node#49211
  - '20.6' not viable due to nodejs/node#49497
  - '20.7' first SES-viable version of Node 20
  - '20.9' first LTS of Node 20
turadg pushed a commit to endojs/endo that referenced this pull request May 20, 2026
@kriskowal @turadg
chore(ci): drop Node.js 20 from the test matrix
e385d46 
Node 20 enters maintenance and is winding down through its LTS
lifecycle. Removing it from the CI matrix narrows the matrix to 18
(still on maintenance), 22 (active LTS), and 24 (current).

The test, cover, test262, and viable-release matrices drop their '20'
entries. Standalone Node-20 jobs (test-xs, test-ocapn-python, and the
ocapn-guile-interop workflow) advance to 22.x rather than being
removed. The test-async-hooks matrix's '20' entry advances to '22'
(the post-promise-fast-path lane on a current LTS, the SES-viable
promise-hook lane the Node-20 entry was guarding). The `test-xs
(macos-15)` lane on Node 20 was filed as flaky.

A historical record of which Node-20 patch versions were SES-viable
is preserved as commentary above the surviving '22' entry in the
test-async-hooks matrix, so future readers see the lineage rather
than floating annotations:

  - '20.3' to '20.6' not viable due to nodejs/node#49211
  - '20.6' not viable due to nodejs/node#49497
  - '20.7' first SES-viable version of Node 20
  - '20.9' first LTS of Node 20
turadg pushed a commit to endojs/endo that referenced this pull request May 20, 2026
@kriskowal @turadg
chore(ci): drop Node.js 20 from the test matrix
790984b 
Node 20 enters maintenance and is winding down through its LTS
lifecycle. Removing it from the CI matrix narrows the matrix to 18
(still on maintenance), 22 (active LTS), and 24 (current).

The test, cover, test262, and viable-release matrices drop their '20'
entries. Standalone Node-20 jobs (test-xs, test-ocapn-python, and the
ocapn-guile-interop workflow) advance to 22.x rather than being
removed. The test-async-hooks matrix's '20' entry advances to '22'
(the post-promise-fast-path lane on a current LTS, the SES-viable
promise-hook lane the Node-20 entry was guarding). The `test-xs
(macos-15)` lane on Node 20 was filed as flaky.

A historical record of which Node-20 patch versions were SES-viable
is preserved as commentary above the surviving '22' entry in the
test-async-hooks matrix, so future readers see the lineage rather
than floating annotations:

  - '20.3' to '20.6' not viable due to nodejs/node#49211
  - '20.6' not viable due to nodejs/node#49497
  - '20.7' first SES-viable version of Node 20
  - '20.9' first LTS of Node 20
turadg pushed a commit to endojs/endo that referenced this pull request May 20, 2026
@kriskowal @turadg
chore(ci): drop Node.js 20 from the test matrix
6cfec20 
Node 20 enters maintenance and is winding down through its LTS
lifecycle. Removing it from the CI matrix narrows the matrix to 18
(still on maintenance), 22 (active LTS), and 24 (current).

The test, cover, test262, and viable-release matrices drop their '20'
entries. Standalone Node-20 jobs (test-xs, test-ocapn-python, and the
ocapn-guile-interop workflow) advance to 22.x rather than being
removed. The test-async-hooks matrix's '20' entry advances to '22'
(the post-promise-fast-path lane on a current LTS, the SES-viable
promise-hook lane the Node-20 entry was guarding). The `test-xs
(macos-15)` lane on Node 20 was filed as flaky.

A historical record of which Node-20 patch versions were SES-viable
is preserved as commentary above the surviving '22' entry in the
test-async-hooks matrix, so future readers see the lineage rather
than floating annotations:

  - '20.3' to '20.6' not viable due to nodejs/node#49211
  - '20.6' not viable due to nodejs/node#49497
  - '20.7' first SES-viable version of Node 20
  - '20.9' first LTS of Node 20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes

@ronag ronag ronag approved these changes

@anonrig anonrig Awaiting requested review from anonrig

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fs Issues and PRs related to the fs subsystem / file system. needs-benchmark-ci PR that need a benchmark CI run. 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.

6 participants

@atlowChemi @nodejs-github-bot @benjamingr @anonrig @ronag @mscdex