leaking!

[dvv]

Hi!

Please, take a look at https://gist.github.com/972495 . Where do we leak?

TIA, --Vladimir

[mranney]

It's not really a leak, you are just publishing faster than the publish commands are going out. You'll need to wire up some kind of throttling mechanism in the command callbacks.

[dvv]

I see. What could it be? For without a feedback on its effect, throttling leads to useless bandwidth loss.

So the commands get stored in the queue somewhere in the client?

[cloudhead]

I think it would just require a 'drain' event.

[dvv]

A special "hiload" publish which sends the data directly on wire, can we have such in the client?

[dvv]

The problem is that under the conditions in this test we constantly call this.command_queue.push(command_obj), but never have chance to shift() in return_reply() (i put a console.log('shift') statement and it got called only once) -- i guess that causes the loss of free memory.

[dvv]

@cloudhead: tried drain -- turned out it's not reliable... https://gist.github.com/972495#file_pub.js

[dvv]

@mranney: could we have a special version of send_command() or otherwise somehow tunable option, to disable queueing command arguments? This would allow to increase throughput of redis pub/sub and avoid memory loss, for some use cases -- of great my concern is broadcasting to a bunch of network nodes. TIA, Vladimir

[mranney]

I will look at why drain wasn't reliable for you. Drain needs to work.

We need some kind of backpressure mechanism though. How would you imagine that a special non-queueing method would work, since it can't be blocking? Would it just drop the message if the queue was too long?

I'm thinking maybe we can adopt the node convention of having the command return true or false depending on whether it just got sent to the underlying socket or not. That way, you could implement a similar pause/drain kind of throttling mechanism.

[dvv]

Would be very welcome. Here is my superpressure testsuite https://gist.github.com/972495 TIA, --Vladimir

[tommedema]

I also welcome a way to tackle this problem.

@dvv, in your drain gist (https://gist.github.com/972495#file_pub.js): did you not forget to pause the process when the buffer is full?

This is the same way piping works with streams, which can be emulated like so:

input.on('data', function(chunk) {
    if (!outputStream.write(chunk)) {
        input.pause();
        outputStream.once('drain', input.resume);
    }
});

I'm not experienced with redis (still looking at the different options), but if this follows the same concept, doesn't your logic lack a "pause" when the buffer is in fact full? Only at such point should you resume operations on a drain event.

[mranney]

I've just pushed up some changes that make it a lot easier to implement different kinds of throttling schemes. Check out this example:

https://github.com/mranney/node_redis/blob/master/examples/backpressure_drain.js

Commands now return false if either the underlying socket write returned false or the command queue high water mark has been met. A drain event will be emitted when the underlying socket drains, or the command queue falls below the low water mark.

[dvv]

great. please, review https://github.com/dvv/node_redis/commit/02c3b01f0e3398c5e701636229e36ce5861233da on how to employ new feature to fight leak

[mranney]

@dvv Can you confirm that this issue is handled as of v0.6.5?

[dvv]

Yes. Thank you very much. We may close this.