Skip to content

Conversation

@himself65
Copy link
Member

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
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
Copy link
Member Author

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

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

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
// There is no shutdown() for files.
WriteStream.prototype.destroySoon = WriteStream.prototype.end;

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

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

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

Development

Successfully merging this pull request may close these issues.

fs: add WriteStream.prototype.fsync

6 participants