Skip to content

fs: add WriteStream.prototype.flush#28519

Closed
himself65 wants to merge 6 commits intonodejs:masterfrom
himself65:28513
Closed

fs: add WriteStream.prototype.flush#28519
himself65 wants to merge 6 commits intonodejs:masterfrom
himself65:28513

Conversation

@himself65
Copy link
Member

@himself65 himself65 commented Jul 3, 2019

fix #28513

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jul 3, 2019
@himself65 himself65 force-pushed the 28513 branch 4 times, most recently from ea56413 to 030dc39 Compare July 3, 2019 09:59
himself65
himself65 commented Jul 3, 2019
View reviewed changes
bnoordhuis
bnoordhuis requested changes Jul 3, 2019
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I appreciate that you took this on but this isn't quite what I had in mind. I tried to explain how it should work but don't hesitate to ask if anything is still unclear.

@himself65 himself65 force-pushed the 28513 branch 7 times, most recently from 54db609 to 14a8cf7 Compare July 3, 2019 16:12
@bnoordhuis bnoordhuis mentioned this pull request Jul 3, 2019
Closed
@himself65 himself65 force-pushed the 28513 branch 5 times, most recently from e4a7187 to 45c2b20 Compare July 4, 2019 05:14
@himself65
Copy link
Member Author

himself65 commented Jul 5, 2019

WIP

@Trott Trott added the wip Issues and PRs that are still a work in progress. label Jul 5, 2019
@himself65
Copy link
Member Author

himself65 commented Jul 7, 2019

I'm trying to refactor the buffer poll to the task poll, which can solve if write chunk or do fsync

@addaleax
Copy link
Member

addaleax commented Jul 7, 2019

@himself65 I’m not sure, but maybe #28520 can provide some inspiration? The code ultimately attempts to solve a very similar problem: Integrate flushing operations (and maybe this function here should also be called flush(), because that may be easier to understand than the POSIX function name) into a stream of to-be-written data. The basic idea there is to use special 0-sized buffers (in this case a single, global 0-sized buffer should suffice), which are then written using .write() and intercepted again in ._write().

@himself65
Copy link
Member Author

himself65 commented Jul 8, 2019

so what is the case of writev?

example:

ws.cork()
ws.write('string')
ws.flush()  // or called ws.fsync()
ws.write('string')
ws.uncork()

in the current will call _writev

@addaleax
Copy link
Member

addaleax commented Jul 8, 2019

@himself65 That’s indeed trickier… it might be okay to perform the fsync operation after all other buffers passed to the _writev() call? Otherwise the logic here would get pretty complex…

@himself65 himself65 changed the title fs: add WriteStream.prototype.fsync fs: add WriteStream.prototype.flush Jul 8, 2019
@himself65 himself65 force-pushed the 28513 branch 2 times, most recently from b65f54e to 3df464d Compare July 8, 2019 12:47
@himself65 himself65 force-pushed the 28513 branch 9 times, most recently from 0610606 to 173061f Compare July 8, 2019 15:40
addaleax
addaleax reviewed Jul 13, 2019
View reviewed changes
lib/internal/fs/streams.js
// There is no shutdown() for files.
WriteStream.prototype.destroySoon = WriteStream.prototype.end;

WriteStream.prototype._flush = function(cb) {
Copy link
Member

@addaleax addaleax Jul 13, 2019

Choose a reason for hiding this comment

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

Fwiw, the naming is unfortunate here, but please be aware that the stream _flush() method does something very different than what the idea for fsync() calls is; it’s called just after the last chunk of data has been written, and doesn’t have anything to do with actually flushing data to the underlying resource (it’s not a great name, definitely).

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jul 7, 2020
@jasnell
Copy link
Member

jasnell commented Jul 7, 2020

@himself65 ... still want to do this?

Converting the PR to a draft given that it's still a work-in-progress

@jasnell jasnell marked this pull request as draft July 7, 2020 14:59
@himself65 himself65 closed this Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax left review comments

@bnoordhuis bnoordhuis bnoordhuis requested changes

Assignees

No one assigned

Labels

fs Issues and PRs related to the fs subsystem / file system. stalled Issues and PRs that are stalled. wip Issues and PRs that are still a work in progress.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fs: add WriteStream.prototype.fsync

6 participants

@himself65 @addaleax @jasnell @bnoordhuis @Trott @nodejs-github-bot