From ed2fc954442707f58fb9857df6435392b4f3c367 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 10 Oct 2015 03:30:53 +0200 Subject: [PATCH] Fix should_buffer return values and empty .batch and .auth return value being sync Fix test --- README.md | 11 ++++++++--- changelog.md | 2 +- index.js | 23 ++++++++++++++--------- test/auth.spec.js | 3 +++ test/{commands => }/batch.spec.js | 22 +++++++++++++++++----- test/commands/multi.spec.js | 17 ++++++++++++++--- 6 files changed, 57 insertions(+), 21 deletions(-) rename test/{commands => }/batch.spec.js (95%) diff --git a/README.md b/README.md index 26ebe09942..9d870b05cc 100644 --- a/README.md +++ b/README.md @@ -152,8 +152,13 @@ So please attach the error listener to node_redis. ### "drain" `client` will emit `drain` when the TCP connection to the Redis server has been buffering, but is now -writable. This event can be used to stream commands in to Redis and adapt to backpressure. Right now, -you need to check `client.command_queue.length` to decide when to reduce your send rate and resume sending commands when you get `drain`. +writable. This event can be used to stream commands in to Redis and adapt to backpressure. + +All commands return a boolean if the stream had to buffer or not. If false is returned the stream had to buffer. +That way you can decide when to reduce your send rate and resume sending commands when you get `drain`. + +You can manually control the low water and high water marks by passing ommand_queue_high_water` and `command_queue_low_water` to the client options. +Check the [Node.js streams API](https://nodejs.org/api/stream.html) for further info. ### "idle" @@ -689,7 +694,7 @@ Client count: 5, node version: 4.1.2, server version: 3.0.3, parser: hiredis End of tests. Total time elapsed: 44952 ms The hiredis and js parser should most of the time be on the same level. The js parser lacks speed for large responses though. -Therefor the hiredis parser is the default used in node_redis. To use `hiredis`, do: +Therefor the hiredis parser is the default used in node_redis and we recommend using the hiredis parser. To use `hiredis`, do: npm install hiredis redis diff --git a/changelog.md b/changelog.md index 4b3ca52303..ce8da25d4a 100644 --- a/changelog.md +++ b/changelog.md @@ -19,7 +19,7 @@ Features Bugfixes - Fix a javascript parser regression introduced in 2.0 that could result in timeouts on high load. ([@BridgeAR](https://github.com/BridgeAR)) -- Fixed should_buffer boolean for .exec, .select and .auth commands not being returned ([@BridgeAR](https://github.com/BridgeAR)) +- Fixed should_buffer boolean for .exec, .select and .auth commands not being returned and fix a couple special conditions ([@BridgeAR](https://github.com/BridgeAR)) If you do not rely on transactions but want to reduce the RTT you can use .batch from now on. It'll behave just the same as .multi but it does not have any transaction and therefor won't roll back any failed commands.
Both .multi and .batch are from now on going to cache the commands and release them while calling .exec. diff --git a/index.js b/index.js index 64b31257ae..f661167f07 100644 --- a/index.js +++ b/index.js @@ -690,7 +690,8 @@ RedisClient.prototype.send_command = function (command, args, callback) { } else { this.emit('error', err); } - return false; + // Singal no buffering + return true; } } @@ -725,7 +726,7 @@ RedisClient.prototype.send_command = function (command, args, callback) { this.offline_queue.push(command_obj); this.should_buffer = true; } - // Return false to signal no buffering + // Return false to signal buffering return false; } @@ -739,7 +740,7 @@ RedisClient.prototype.send_command = function (command, args, callback) { err = new Error('Connection in subscriber mode, only subscriber commands may be used'); err.command = command.toUpperCase(); this.emit('error', err); - return false; + return true; } this.command_queue.push(command_obj); this.commands_sent += 1; @@ -800,7 +801,7 @@ RedisClient.prototype.writeStream = function (data) { // Do not use a pipeline if (this.pipeline === 0) { - return !this.stream.__write(data); + return this.stream.__write(data); } this.pipeline--; this.pipeline_queue.push(data); @@ -975,11 +976,13 @@ RedisClient.prototype.auth = RedisClient.prototype.AUTH = function (pass, callba var err = new Error('The password has to be of type "string"'); err.command = 'AUTH'; if (callback) { - callback(err); + setImmediate(function () { + callback(err); + }); } else { this.emit('error', err); } - return false; + return true; } this.auth_pass = pass; debug('Saving auth as ' + this.auth_pass); @@ -988,7 +991,7 @@ RedisClient.prototype.auth = RedisClient.prototype.AUTH = function (pass, callba return this.send_command('auth', [this.auth_pass], callback); } this.auth_callback = callback; - return false; + return true; }; RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function (key, args, callback) { @@ -1180,9 +1183,11 @@ Multi.prototype.exec = Multi.prototype.EXEC = function (callback) { var args; if (len === 0) { if (callback) { - callback(null, []); + setImmediate(function () { + callback(null, []); + }); } - return false; + return true; } this.results = new Array(len); this._client.pipeline = len; diff --git a/test/auth.spec.js b/test/auth.spec.js index b4646c1fdc..ef34b82ccb 100644 --- a/test/auth.spec.js +++ b/test/auth.spec.js @@ -128,12 +128,15 @@ describe("client authentication", function () { if (helper.redisProcess().spawnFailed()) this.skip(); client = redis.createClient.apply(redis.createClient, args); + var async = true; client.auth(undefined, function(err, res) { assert.strictEqual(err.message, 'The password has to be of type "string"'); assert.strictEqual(err.command, 'AUTH'); assert.strictEqual(res, undefined); + async = false; done(); }); + assert(async); }); it('should emit an error if the password is not of type string and no callback has been provided', function (done) { diff --git a/test/commands/batch.spec.js b/test/batch.spec.js similarity index 95% rename from test/commands/batch.spec.js rename to test/batch.spec.js index 0e009c5b21..5164d897e9 100644 --- a/test/commands/batch.spec.js +++ b/test/batch.spec.js @@ -1,8 +1,8 @@ 'use strict'; var assert = require('assert'); -var config = require("../lib/config"); -var helper = require('../helper'); +var config = require("./lib/config"); +var helper = require('./helper'); var redis = config.redis; var uuid = require('uuid'); @@ -52,7 +52,7 @@ describe("The 'batch' method", function () { beforeEach(function (done) { client = redis.createClient.apply(redis.createClient, args); - client.once("connect", function () { + client.once("ready", function () { client.flushdb(function (err) { return done(err); }); @@ -63,6 +63,19 @@ describe("The 'batch' method", function () { client.end(); }); + it("returns an empty result array", function (done) { + var batch = client.batch(); + var async = true; + var notBuffering = batch.exec(function (err, res) { + assert.strictEqual(err, null); + assert.strictEqual(res.length, 0); + async = false; + done(); + }); + assert(async); + assert.strictEqual(notBuffering, true); + }); + it('fail individually when one command fails using chaining notation', function (done) { var batch1, batch2; batch1 = client.batch(); @@ -216,8 +229,7 @@ describe("The 'batch' method", function () { it('runs a batch without any further commands and without callback', function() { var buffering = client.batch().exec(); - assert(typeof buffering === 'boolean'); - assert(!buffering); + assert.strictEqual(buffering, true); }); it('allows batchple operations to be performed using a chaining API', function (done) { diff --git a/test/commands/multi.spec.js b/test/commands/multi.spec.js index 99e43d472f..73aaa46bf4 100644 --- a/test/commands/multi.spec.js +++ b/test/commands/multi.spec.js @@ -32,11 +32,12 @@ describe("The 'multi' method", function () { }); it("reports an error", function (done) { - client.multi(); - client.exec(function (err, res) { + var multi = client.multi(); + var notBuffering = multi.exec(function (err, res) { assert(err.message.match(/The connection has already been closed/)); done(); }); + assert.strictEqual(notBuffering, false); }); it("reports an error if promisified", function () { @@ -51,7 +52,7 @@ describe("The 'multi' method", function () { beforeEach(function (done) { client = redis.createClient.apply(redis.createClient, args); - client.once("connect", function () { + client.once("ready", function () { client.flushdb(function (err) { return done(err); }); @@ -62,6 +63,16 @@ describe("The 'multi' method", function () { client.end(); }); + it("returns an empty result array", function (done) { + var multi = client.multi(); + var notBuffering = multi.exec(function (err, res) { + assert.strictEqual(err, null); + assert.strictEqual(res.length, 0); + done(); + }); + assert.strictEqual(notBuffering, true); + }); + it('roles back a transaction when one command in a sequence of commands fails', function (done) { var multi1, multi2; var expected = helper.serverVersionAtLeast(client, [2, 6, 5]) ? helper.isError() : function () {};