Skip to content

fs: add WriteStream.prototype.flush - #28519

Closed
himself65 wants to merge 6 commits into
nodejs:masterfrom
himself65:28513
Closed

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

Conversation

@himself65

Copy link
Copy Markdown
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-botnodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jul 3, 2019
@himself65
himself65force-pushed the 28513 branch 4 times, most recently from ea56413 to 030dc39CompareJuly 3, 2019 09:59
Comment threadlib/internal/fs/streams.js Outdated

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

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.

Comment threadtest/parallel/test-fs-write-stream-fsync.js Outdated
Comment threadlib/internal/fs/streams.js Outdated
@himself65
himself65force-pushed the 28513 branch 7 times, most recently from 54db609 to 14a8cf7CompareJuly 3, 2019 16:12
@himself65
himself65force-pushed the 28513 branch 5 times, most recently from e4a7187 to 45c2b20CompareJuly 4, 2019 05:14
@himself65

Copy link
Copy Markdown
MemberAuthor

WIP

@TrottTrott added the wip Issues and PRs that are still a work in progress. label Jul 5, 2019
@himself65

Copy link
Copy Markdown
MemberAuthor

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

@addaleax

Copy link
Copy Markdown
Member

@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
Copy Markdown
MemberAuthor

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
Copy Markdown
Member

@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…

@himself65himself65 changed the title fs: add WriteStream.prototype.fsync fs: add WriteStream.prototype.flushJul 8, 2019
@himself65
himself65force-pushed the 28513 branch 2 times, most recently from b65f54e to 3df464dCompareJuly 8, 2019 12:47
@himself65
himself65force-pushed the 28513 branch 9 times, most recently from 0610606 to 173061fCompareJuly 8, 2019 15:40
// There is no shutdown() for files.
WriteStream.prototype.destroySoon = WriteStream.prototype.end;

WriteStream.prototype._flush = function(cb) {

Copy link
Copy Markdown
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
BridgeARforce-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72CompareMay 31, 2020 12:19
@jasnelljasnell added the stalled Issues and PRs that are stalled. label Jul 7, 2020
@jasnell

Copy link
Copy Markdown
Member

@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
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fsIssues and PRs related to the fs subsystem / file system.stalledIssues and PRs that are stalled.wipIssues 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

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