Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set highWaterMark to 16 in objectMode #18

Merged
merged 1 commit into from
Jun 4, 2014
Merged

Conversation

mafintosh
Copy link
Collaborator

Currently in node 0.10 hwm defaults to ~16000 objects when streaming in objectMode which means that backpressure will almost never happen.

This is fixed in node 0.12 where it will default to 16 in objectMode.
We can fix this now in through2 by merging this PR :)

@Delapouite
Copy link
Collaborator

Here's the associated fix in node core from last August :

nodejs/node-v0.x-archive@ba72570

rvagg added a commit that referenced this pull request Jun 4, 2014
Set highWaterMark to 16 in objectMode
@rvagg rvagg merged commit 68fde57 into rvagg:master Jun 4, 2014
@rvagg
Copy link
Owner

rvagg commented Jun 4, 2014

@0.5.0 published, thanks @mafintosh

@maxkueng
Copy link

Do I have to make any changes to my code because of this?

I have this through2 object stream. The source is a LevelDB value stream (JSON). When I pipe it through the stream below it only processes exactly 16 objects, then stops and never triggers end.

var full = through2.obj(function (data, enc, callback) {
  geojson.geometry.coordinates.push([ data.longitude, data.latitude, data.altitude ]);

  this.push(data);
  callback();
});

@rvagg
Copy link
Owner

rvagg commented Jun 19, 2014

@maxkueng are you piping it somewhere else? the this.push(data) is going to queue it up and unless you have a stream on the other end of it it'll just buffer and then stop when it reaches 16.

I wonder if perhaps you're terminating on this stream and therefore don't want to be doing the this.push(data) at all, if you remove it then it'll make it a terminating stream and the callback() will keep things flowing because there is nothing to buffer.

@maxkueng
Copy link

@rvagg Wow, very fast reply skills! Thanks! I removed this.push(data) and it seems to work. 👍

Do I understand correctly? The last stream I pipe through must be a writable stream or a through2 without this.push.

@rvagg
Copy link
Owner

rvagg commented Jun 19, 2014

yeah, you need your stream to terminate somewhere, usually with just a Writable stream that pushes it into the system (filesystem, network) or some kind of processing terminator. If you this.push() then you're queuing up the data in the internal buffer where it'll wait for a for a stream to be piped in to, which can be useful but not if you're intending the data to end there. What you're doing is making a proper Duplex stream by adding data, so your Writable is also becoming a Readable but that means the data needs to be read by something.

Also see https://github.com/brycebaril/node-terminus#terminusdevnulloptions for another option, that case is simply a through2 stream that does nothing, so if you did want to this.push(data) to make a duplex stream then you could pipe to a terminator (devnull) stream that does nothing but consume your data. This can be handy if you're using a stream that is intended to be piped elsewhere but your really just want to end processing there.

For example, perhaps you wanted to use count-stream to do some counting but that's all you want to do with the data. count-stream is duplex and is intended to be placed in the middle of a processing chain, to make the data keep flowing you'd need to pipe that to a terminator like devnull.

@maxkueng
Copy link

It makes totally sense now. I guess I just never ran in to the issue before where I didn't have a sink/terminating stream at the end. I also thought this.push would just send the data to nirvana if it was the last stream. Now I know :)

Thank you very much for the detailed reply and the module tips. Very appreciated.

@eddieajau
Copy link

yeah, you need your stream to terminate somewhere

@rvagg is it worth adding (I'm happy to) this little gem to the README? I just spent a few hours trying to figure out what was going wrong in my implementation and this was the problem.

@rvagg
Copy link
Owner

rvagg commented Nov 19, 2016

@eddieajau yeah, you could, it trips people up all the time although it's not specific to through2 but we could help educate at least.

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

Successfully merging this pull request may close these issues.

None yet

5 participants