fs: WriteStream should handle partial writes #22740
Conversation
nodejs-github-bot
added
the
fs
lib/internal/fs/streams.js
Outdated
| if (er) { | ||
| if (this.autoClose) { | ||
| this.destroy(); | ||
| } | ||
| return cb(er); | ||
| } | ||
| this.bytesWritten += bytes; | ||
| cb(); | ||
| }); | ||
| pos += bytes; |
There was a problem hiding this comment.
Note that pos may be undefined if this.pos was undefined. It would be better to pass undefined to fs.write rather than NaN in this case.
|
/cc @nodejs/fs |
|
Thanks for addressing this issue! Makes you wonder... what happens with |
|
@kyliau Can you look into @ronkorving’s comment? I think |
|
Also, how does libuv/libuv#1742 fit into this? I haven’t dug much but from the sound of it it seems like it would fix the same issue on the libuv level? |
|
@ronkorving I briefly looked at @addaleax It's true that if the underlying medium is a HDD and it's full then we shouldn't continue writing. The case where I encountered this bug is in a Unix pipe, where partial write occurred when the payload is much larger than the pipe buffer, but it's ok to resume writing once the pipe is drained. In either case, the user has no way of knowing that a partial write occurred. Would it be better to pass along the written bytes to the callback so user is aware and can take appropriate steps? |
Given a buffer of length l, fs.write() will not necessarily write the entire buffer to the file. This can occur if, for example, there is insufficient space on the underlying physical medium. WriteStream did not handle this case, and when partial write occurs, it will errorneously report that the write is successful. This commit changes the _write() behavior to continue the write operation, picking up from where the last operation left off. More information about the write() system call is available at http://man7.org/linux/man-pages/man2/write.2.html
|
@gireeshpunathil reported similar issue with an example here |
|
FYI: libuv/libuv#1742 has just landed. |
Given a buffer of length l, fs.write() will not necessarily write the
entire buffer to the file. This can occur if, for example, there is
insufficient space on the underlying physical medium.
WriteStream did not handle this case, and when partial write occurs,
it will errorneously report that the write is successful.
This commit changes the _write() behavior to continue the write
operation, picking up from where the last operation left off.
More information about the write() system call is available at
http://man7.org/linux/man-pages/man2/write.2.html
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes