Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

Streams: Add _flush capability to writable streams #7613

Closed
wants to merge 1 commit into from

Conversation

evansolomon
Copy link

Allows writable streams to define a _flush() method that can delay the finish event until it completes.

See #7348

@@ -126,12 +126,7 @@ function Transform(options) {
this._readableState.sync = false;

this.once('prefinish', function() {

Choose a reason for hiding this comment

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

It looks like there is no need in prefinish after this change

@vkurchatkin
Copy link

@markstos is right and this doesn't work for async _flush:

  w._flush = function(cb){
    process.nextTick(function () {
      writtenData.push('flushed');
      cb();
    });
  }

I don't think it's a bug with current Transform implementation, but it's definitely a bug for Writable

writtenData.push(chunk.toString());
cb();
}
w._flush = function(cb){

Choose a reason for hiding this comment

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

Also try an a async implemention of _flush and see if it still completes
before the 'finish' event is fired:

w._flush = function(cb){
 process.nextTick(function () {
  writtenData.push('flushed');
   cb();
 }); 
};

See related discussion here:
#7612

@calvinmetcalf
Copy link

took a stab at this as well (haven't opened a pull, but can the branch is at https://github.com/calvinmetcalf/node/tree/write-flush I can open my own pull here but was waiting until it actually worked), as it is it fails 2 of the tests

  • simple/test-stream2-writable#' finish does not come before sync _write cb' fails pretty strait forwardly
  • simple/test-zlib-params fails due to the following bytes 01 00 00 ff ff a5 ac 3d 34 being appended to the output (wtf?)

@jasnell
Copy link
Member

jasnell commented Aug 13, 2015

@chrisdickinson @trevnorris ... this never seems to have been completed. Any thoughts on whether this is needed and whether it would need to land in v0.12?

@calvinmetcalf
Copy link

there is a more comlete pull to do this at nodejs/node#2314

@jasnell
Copy link
Member

jasnell commented Aug 13, 2015

Excellent. Closing this then in favor of that. Can reopen this if it becomes necessary

@jasnell jasnell closed this Aug 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants