gulpjs / async-done

Allows libraries to handle various caller provided asynchronous functions uniformly. Maps promises, observables, child processes and streams, and callbacks to callback style.
MIT License
70 stars 21 forks source link

Drain unpiped readable streams? #11

Closed pkozlowski-opensource closed 10 years ago

pkozlowski-opensource commented 10 years ago

So, I've got impression that we need to do the same fix as in the original orchestrator as discussed here: https://github.com/orchestrator/orchestrator/issues/48#issuecomment-49504958

Here is a simple reproduce scenario:

var Readable = require('stream').Readable;

var through = require('through2');
var asyncDone = require('async-done');

var rs = new Readable({objectMode: true});
var c = 0;
rs._read = function () {
  if (c++ < 100) {
    rs.push(c);
  } else {
    rs.push(null);
  }
};

asyncDone(function(){

  return rs.pipe(through.obj(function (chunk, enc, cb) {
    console.log(chunk);
    this.push(chunk);
    cb();
  }));

}, function(err, result){
  console.log(err, result);
});

In this scenario the node process will terminate after reading 16 items.

Now that we've got stream-consume as a separate module a fix would be trivial, happy to send a PR if this makes sense.

phated commented 10 years ago

stream-consume is a very good module. I was thinking that I wouldn't be adding any consuming into gulp4 and that people should end their streams with one that consumes, but with that module, it seems like we could add it. Do we add this to async-done, bach, or orchestrator?

pkozlowski-opensource commented 10 years ago

Yeh, I kind of agree that people should consume the stream on their end but it is super-non-obvious that is should be done. As such many people will be left wondering what is going on... I usually don't like to adding those conveniences as people should (normally) understand what they are dealing with but yeh, this one is really not obvious.

For me async-done was kind of natural place as this is where we detect that we deal with a stream already but I don't think I've got complete mental picture of all the elements to judge if this is the best place or not....

phated commented 10 years ago

async-done is the most general library that lets someone transform any API into a node-style callback API.

pkozlowski-opensource commented 10 years ago

@phated I was indeed concerned with the fact that draining the stream isn't really async-done responsibility and reduces its general-purpose usage. Having said this I can't even think of any solution that would allow shift this responsibility to the orchestrator (IMO this is the only place besides async-done where it could makes sense).... I will dig into this some more during the weekend but i'm curious what would be your take on assigning stream-draining responsibility. Where would you put it?

phated commented 10 years ago

I wonder if we could somehow build this into orchestrator along with the timing stuff. The only problem with that would be duplicating the return type checks.

phated commented 10 years ago

Using https://github.com/chrisdickinson/stream-exhaust, I was able to implement this inside async-done. Thanks @chrisdickinson!