fs: make sure to write entire buffer by ronag · Pull Request #42434 · nodejs/node

ronag · 2022-03-22T11:49:41Z

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

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

lib/internal/fs/streams.js

benjamingr · 2022-03-22T13:27:36Z

lib/internal/fs/streams.js

+    retries = bytesWritten ? 0 : retries + 1
+
+    if (retries > 5) {
+      return cb(new Error('writev failed'));


What are we trying for 5 times here? Writing a non-zero number of bytes?

Maybe add a comment here.

mcollina · 2022-03-22T13:40:11Z

lib/internal/fs/streams.js

+    retries = bytesWritten ? 0 : retries + 1
+
+    if (retries > 5) {
+      return cb(new Error('writev failed'));


Maybe add a comment here.

mcollina · 2022-03-22T13:41:44Z

lib/internal/fs/streams.js

+    }
+
+    if (bytesWritten < size) {
+      writevAll([Buffer.concat(buffers).slice(bytesWritten)], pos + bytesWritten, cb, retries);


I would recommend being a bit more subtle here and avoid the massive slowdown. Worst case you are allocating 2x memory to pick one byte. I would recommend just concatenating the buffers that have not been completely written.

This is a cold path though? It should almost never happen. Is it worth optimizing for?

ronag · 2022-03-22T13:45:07Z

lib/internal/fs/streams.js

+    pos += bytesWritten;
+
+    if (retries > 5) {
+      cb(new Error('writev failed'));


I need a little help here... not sure what error to use

Lemme check

Checked the existing codes, no idea either 😅

mscdex · 2022-03-22T14:33:15Z

lib/internal/fs/streams.js

+    if (retries > 5) {
+      cb(new Error('writev failed'));
+    } else if (size) {
+      writevAll([Buffer.concat(buffers).slice(bytesWritten)], size, pos, cb, retries);


If we're going to be concatenating, we may as well just do it once and call writeAll() with it instead. Otherwise like @mcollina said, we should handle this more efficiently.

Ideally it would be great if we didn't have to even keep recreating arrays on retry. That might be a nice addition to the fs.writev() API to make it more like fs.write() with its offset parameter, so we could just pass a starting index or something. With something like that all we'd have to do is possibly slice() a single buffer (or simply increasing the starting index) before retrying.

This is a cold path though? It should almost never happen. Is it worth optimizing for?

It should almost never happen

It think it would be useful to get an idea of how often it happens (I personally have no idea). How cold is it?

Well, if it does happen often then we have a lot of seriously broken software out there... I guess we would have quite a bit of reports....

or people have learned to handle retries on their own?

I don’t think you can actually detect that in this interface… there is nothing a user can do… it will just be corrupt w/o any way to detect or recover… so no

Well, if it does happen often then we have a lot of seriously broken software out there... I guess we would have quite a bit of reports....

makes sense, yeah sounds like a cold path to me.

mscdex · 2022-03-22T14:37:45Z

Needs a test as well.

benjamingr · 2022-03-22T15:16:07Z

lib/internal/fs/streams.js

+    pos += bytesWritten;
+
+    if (retries > 5) {
+      cb(new Error('writev failed'));


This needs to be a proper error with code?

Suggestions?

lib/internal/fs/promises.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

evanlucas · 2022-04-25T11:26:21Z

lib/internal/fs/streams.js

+    }
+
+    if (this.destroyed || er) {
+      return cb(er);


will er here be something if this.destroyed is truthy? Otherwise, this could signal to the caller that the call succeeded?

ronag · 2022-10-27T10:31:16Z

@nodejs/fs someone willing to help get this over the finish line? I still think it's a potential bug.

benjamingr · 2023-08-14T10:02:56Z

Maybe @rluvaton or @atlowChemi ?

rluvaton · 2023-08-14T20:21:55Z

What help is needed?

ronag · 2023-08-14T21:08:23Z

Linting and Tests

atlowChemi · 2023-08-16T20:45:04Z

@nodejs/fs someone willing to help get this over the finish line? I still think it's a potential bug.

@ronag Can you check the Allow edits and access to secrets by maintainers?
If there is another way for me to push changes to the branch let me know 🙂

ronag · 2023-08-17T07:57:55Z

@nodejs/fs someone willing to help get this over the finish line? I still think it's a potential bug.

@ronag Can you check the Allow edits and access to secrets by maintainers? If there is another way for me to push changes to the branch let me know 🙂

Hm. I don't know how to do that. Can you just push to a new branch/PR?

aduh95 · 2023-09-20T08:29:58Z

Superseded by #49211

fs: make sure to write entire buffer

4307397

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

ronag added the fs Issues and PRs related to the fs subsystem / file system. label Mar 22, 2022

ronag requested review from addaleax, jasnell and mcollina March 22, 2022 11:49

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 22, 2022

ronag commented Mar 22, 2022

View reviewed changes

lib/internal/fs/streams.js Show resolved Hide resolved

mcollina reviewed Mar 22, 2022

View reviewed changes

lib/internal/fs/streams.js Show resolved Hide resolved

lib/internal/fs/streams.js Show resolved Hide resolved

fixup

b6b9680

ronag requested a review from mcollina March 22, 2022 13:18

ronag added 3 commits March 22, 2022 14:23

fixup

a5bf600

fixup

523314a

fixup

d4c7a90

ma1971001 reviewed Mar 22, 2022

View reviewed changes

lib/internal/fs/streams.js Show resolved Hide resolved

benjamingr reviewed Mar 22, 2022

View reviewed changes

nodejs blocked ma1971001 Mar 22, 2022

ronag added 4 commits March 22, 2022 14:35

fixuP

d0afc94

fixuP

43f2529

fixuP

4ff89b4

fixuP

3869561

mcollina reviewed Mar 22, 2022

View reviewed changes

ronag added 3 commits March 22, 2022 14:42

fixuP

ab95708

fixuP

43d9020

fixuP

09b7994

ronag commented Mar 22, 2022

View reviewed changes

mscdex reviewed Mar 22, 2022

View reviewed changes

benjamingr reviewed Mar 22, 2022

View reviewed changes

mscdex added the needs-benchmark-ci PR that need a benchmark CI run. label Mar 22, 2022

aduh95 reviewed Mar 22, 2022

View reviewed changes

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

Update lib/internal/fs/promises.js

be950ef

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

evanlucas reviewed Apr 25, 2022

View reviewed changes

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

atlowChemi mentioned this pull request Aug 17, 2023

fs: make sure to write entire buffer #49211

Closed

aduh95 closed this Sep 20, 2023

Uh oh!

Conversation

ronag commented Mar 22, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronag Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Mar 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronag commented Oct 27, 2022

Uh oh!

benjamingr commented Aug 14, 2023

Uh oh!

rluvaton commented Aug 14, 2023

Uh oh!

ronag commented Aug 14, 2023

Uh oh!

atlowChemi commented Aug 16, 2023

Uh oh!

ronag commented Aug 17, 2023

Uh oh!

aduh95 commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

mscdex Mar 22, 2022 •

edited

Loading

mscdex Mar 22, 2022 •

edited

Loading

ronag Mar 22, 2022 •

edited

Loading