Propogate a drain event when no resume method is defined

[mikeal]

Paused pipe() chains will never resume in 0.6 if a stream is in the chain that uses stream.Stream and doesn't define it's own resume() method.

[koichik]

If the readable stream must implement resume(), this patch is not needed.
The stream API has some contracts, why is not resume() a must?

[mikeal]

yes, and it's stream.Stream's job to implement that contract :)

we'll have a much more comprehensive fix once we extend stream.Stream but that's not happening until 0.7. we should have done that before removing the current pause() and resume() semantics but ryan decided to move forward in 0.6.

at the very least we need to un-break 0.6 drain.

[koichik]

The streams API has pause() and resume() since it was introduced with Node v0.1.90. On the other hand, 'pasue' and 'resume' have never been documented.
And, because all source streams are not a filter, it is strange to emit 'drain' event on the source stream.

So, if the readable stream does not have resume(), I think that it should implement resume().
We can write it in the API changes if necessary.

[mikeal]

@koichik that's a great fix for 0.7 but in 0.6 i would prefer this because

1. it matches what we did in prior releases
2. the value add in stream.Stream hasn't been very large yet so there are lots of userland streams that don't use it as a base, fixing it outside of the pipe() method in the object doesn't do much for them.

this would be different if we had time to make the improvements that we want to stream.Stream but we haven't, in fact we've only removed functionality and broke things in stream.Stream in this release so i don't know why developers would suddenly start listening to us and using it as a base when we have nothing new to offer them.

[mikeal]

i'm getting a little frustrated.

are we literally saying that, this used to work, now it doesn't, and the fix is to add documentation to tell people to write a stub method we used to have working? cause if it is, that's bullshit.

[isaacs]

@koichik I actually think @mikeal's right on this. The stream api is in a somewhat mixed up state, but there are real userland streams that worked in 0.4 and don't work in 0.6 as a result of removing the pause/resume propagation, because they relied on the default behavior to send the resume message up the chain.

This change would make that work by passing a drain message up the chain for filter streams that don't implement resume themselves. It's not really a satisfactory answer to say "well, they need to follow the contract," because that contract was never fully fleshed out or documented. What we did was present a thing that works, and then break it.

+1 for this change, if @mikeal can add some indentation and a semicolon to make it pass jslint ;)

[mikeal]

f'ing semicolons.

[koichik]

Okay, I do not block this any longer.

[mikeal]

can we get some committer attention on this?

[Nodejs-Jenkins]

Can one of the admins verify this patch?

[isaacs]

Fixed this by rewriting the entire streams interface (twice)