Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

streams2 #106

Closed
rvagg opened this issue Mar 22, 2013 · 12 comments
Closed

streams2 #106

rvagg opened this issue Mar 22, 2013 · 12 comments

Comments

@rvagg
Copy link
Member

rvagg commented Mar 22, 2013

Inherit from readable-stream I guess. Or whatever's best for object-streams (I can't keep up!).

For both ReadStream and WriteStream.

WriteStream needs some love and it may be best to just redo it entirely. I'd be comfortable with removing fstream-compatibility; it's always been just a novelty really and I doubt anyone is actually using it.

@Raynos
Copy link
Member

Raynos commented Mar 22, 2013

https://github.com/Raynos/level-write-stream/blob/master/index.js

I already wrote one of those. It should just require("readable-stream").Writable instead and handle actual edge cases

@rvagg
Copy link
Member Author

rvagg commented Mar 22, 2013

@Raynos I might look at pulling yours into core if you don't mind; it looks like it might do it all minus the fstream stuff (which probably belongs in an external package if it's needed at all).

@Raynos
Copy link
Member

Raynos commented Mar 23, 2013

@rvagg pulling it into core is fine. We can also just take read & write streams and make them seperate modules that levelup depends on

@rvagg
Copy link
Member Author

rvagg commented Mar 23, 2013

True, WriteStream is probably the best candidate for that since it doesn't need anything special from core; it's pure sugar. I'll have to think more about ReadStream because it has to hook into iterators and I'm not sure I want to make iterators an official part of the LevelUP API.

@dominictarr
Copy link
Contributor

Another option is just to bundle it into core -- and Raynos can add who he needs to the repo.

While I am here, iterators are pretty sweet,
because they are so minimal - and can easily be wrapped into any streaming abstraction.

But, I guess if you want iterators, you can use leveldown.

@rvagg
Copy link
Member Author

rvagg commented Mar 23, 2013

I guess if we remove fstream from ReadStream and make it accept an iterator on the constructor then it'd be easy enough to extract as a separate lib and make it generic for the iterator pattern.

@mirkokiefer
Copy link

@dominictarr I actually find the async iterator pattern much more convenient for database traversal. The problem with leveldown is that it doesn't protect you from crashing your process - thats all managed by levelup.
So I had to write levelup-iterator which creates an iterator from levelup's readable stream.

@dominictarr
Copy link
Contributor

@mirkokiefer leveldown is exposed in levelup, as db.db it's probably better to use that directly than to polyfil back to the primitive.

Also, if leveldown can be made safe without impacting performance, then we should fix those problems - what are the problematic cases with leveldown?

@mirkokiefer
Copy link

yes, it would definitely be cleaner if levelup exposed a safe version of an iterator and the readable stream would be built on top of that.
The safety issues in leveldown are described here:
https://github.com/rvagg/node-leveldown#safety

Looking at the readable stream tests in levelup it looks like there are in fact some nasty corner cases that need to be considered - so for my use case it was much easier to simply wrap the stream in an iterator.

@rvagg
Copy link
Member Author

rvagg commented Jul 7, 2013

Actually, I should update that section because leveldown now tracks open iterators and closes them when you close the db. Using LevelDOWN iterators directly should be safe and if you run into any issues then report them against leveldown and we'll try and address them (or you could try and address them yourself of course!).

As @dominictarr said you can access the iterator API directly even if you're using LevelUP:

> var l = require('levelup')
> var db = l('/tmp/db')
> db.readStream().on('data', console.log)
{ key: 'foo2', value: 'bar' }
{ key: 'foo3', value: 'bar' }
{ key: 'foo4', value: 'bar' }
{ key: 'foo5', value: 'bar' }
> var it = db.db.iterator()
> it.next(function (err, value) { console.log(err, value && value.toString()) })
null 'foo'
> it.next(function (err, value) { console.log(err, value && value.toString()) })
null 'foo2'
> it.next(function (err, value) { console.log(err, value && value.toString()) })
null 'foo3'
> it.next(function (err, value) { console.log(err, value && value.toString()) })
null 'foo4'
> it.next(function (err, value) { console.log(err, value && value.toString()) })
null 'foo5'
> it.next(function (err, value) { console.log(err, value && value.toString()) })
undefined undefined

@mirkokiefer
Copy link

Great, I will try using them directly and let you know when I run into issues!
Are put/get operations now safe in leveldown as well? Because then I could actually use leveldown straight away...

@rvagg
Copy link
Member Author

rvagg commented Jul 7, 2013

Yes, leveldown has matured somewhat and is generally pretty safe; the only major concern is managing state, you need to care more about when it's open & closed, there's no deferred-open like levelup offers you and it's a bit more strict about arguments. Apart from all of that it's pretty solid and if you run into any problems, particularly anything that lets you crash the Node process then report it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants