using streams2 for db.createReadStream()

[ghost]

This patch uses readable-stream.Readable for the db.createReadStream() api to implement the readable half of issue #106. Only a few minor tweaks to the existing test suite were necessary to work around a few streams2isms.

This patch also fixes the issue where calling .pause() will let one more element through before pausing and I was running into some issues with piping to multiple destinations that this patch also seems to have fixed.

[ghost]

Here's the benchmark results from running test/benchmark/stream-bench.js:

BEFORE (current master, 75104e0):

$ node stream-bench.js 
--------------------------------------------------------------
Filled source! Took 34850ms, streaming to destination...
Done! Took 46254ms, 133% of batch time

AFTER (this patch, bc791ba):

$ node stream-bench.js 
--------------------------------------------------------------
Filled source! Took 34457ms, streaming to destination...
Done! Took 39947ms, 116% of batch time

That's a 17% speedup from the old code to this streams2 patch. Hooray!

[mcollina]

This is super good. Definitely 👍

[dominictarr]

why does close happen before end?
do you mean immediately before end?
or at some point before end?

[kanongil]

@dominictarr That is a streams2ism. It will happen at some point before end, caused by internal buffering.

[brycebaril]

readable-stream doesn't check to see if you have Readable from core (node 0.9+) -- it was recently updated to be the same as 0.10.15, but it doesn't stay version-locked. You probably want to check for core support first.

I.e.
var Readable = require('stream').Readable || require('readable-stream').Readable

[rvagg]

+1

[rvagg]

these can go, as can the test case cruft for them

[rvagg]

LGTM

The changes to the test cases around close/end are slightly concerning and I don't fully understand what's going on there, however, I'm pretty happy that we're pushing responsibility for maintaining state etc. to Readable so our tests don't need to be quite as thorough around that stuff.

Anyone else got anything to add on this? Otherwise I guess it can be merged when these minor things are fixed up. @substack are you happy to become one of the maintainers? See the discussions on this when we added our last maintainer, @pgte: #160 (comment)

[rvagg]

and while you're at it, you can remove simple-bufferstream since it's only there to support fstream compatibility in the ReadStream.

[ghost]

The close/end changes are due to how with streams 2 there is no way of knowing from the producer when a readable stream was done being read because of internal buffering and how with streams 2 you can't .on('end', fn) unless the stream is in flow/classic mode. The only thing we know is when we sent the last chunk, which is when the 'close' event fires. The assumption in the tests was that 'end' would always come before 'close' but this can't be the case in streams2.

I'll become a maintainer, sure! I don't plan on doing anything differently however as I'll still send pull requests.

[dominictarr]

@substack so how do you know when the stream has finished, is that the 'finish' event?

[ghost]

Only the consumer knows when the stream is finished. There is no 'finish' event.

[dominictarr]

this looks like an end event to me...
https://github.com/joyent/node/blob/master/lib/_stream_readable.js#L485

[rvagg]

publishing a @0.15.0 with this and linking to leveldown @0.8.0 (which has the new gt/gte/lt/lte stuff and nan as a dependency).

no docs yet for gt/gte/lt/lte but it's there, along with start/end

adding @substack as a maintainer, 👏