From 575ad7357b5f51bc02209e7fecdedb26f7883024 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 17:11:02 +0100 Subject: [PATCH] Insert deprecation warnings and some minor refactoring --- README.md | 19 ++++---- index.js | 91 ++++++++++++++++++++++++++---------- test/commands/blpop.spec.js | 8 ++++ test/commands/get.spec.js | 1 - test/commands/getset.spec.js | 1 - test/connection.spec.js | 2 + test/lib/stunnel-process.js | 7 ++- test/node_redis.spec.js | 32 +++++++++---- test/utils.spec.js | 25 ++++++++++ 9 files changed, 140 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 1de7668..c48054b 100644 --- a/README.md +++ b/README.md @@ -147,7 +147,7 @@ So please attach the error listener to node_redis. `client` will emit `end` when an established Redis server connection has closed. -### "drain" +### "drain" (deprecated) `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. @@ -162,7 +162,7 @@ If false is returned the stream had to buffer. `client` will emit `warning` when password was set but none is needed and if a deprecated option / function / similar is used. -### "idle" +### "idle" (deprecated) `client` will emit `idle` when there are no outstanding commands that are awaiting a response. @@ -170,12 +170,13 @@ If false is returned the stream had to buffer. If you have `redis-server` running on the same computer as node, then the defaults for port and host are probably fine and you don't need to supply any arguments. `createClient()` returns a `RedisClient` object. +If the redis server runs on the same machine as the client consider using unix sockets if possible to increase throughput. + ### overloading -* `redis.createClient()` -* `redis.createClient(options)` -* `redis.createClient(unix_socket, options)` -* `redis.createClient(redis_url, options)` -* `redis.createClient(port, host, options)` +* `redis.createClient([options])` +* `redis.createClient(unix_socket[, options])` +* `redis.createClient(redis_url[, options])` +* `redis.createClient(port[, host][, options])` #### `options` is an object with the following possible properties: * `host`: *127.0.0.1*; The host to connect to @@ -204,10 +205,10 @@ This delay normally grows infinitely, but setting `retry_max_delay` limits it to * `connect_timeout`: *3600000*; Setting `connect_timeout` limits total time for client to connect and reconnect. The value is provided in milliseconds and is counted from the moment on a new client is created / a connection is lost. The last retry is going to happen exactly at the timeout time. Default is to try connecting until the default system socket timeout has been exceeded and to try reconnecting until 1h passed. -* `max_attempts`: *0*; By default client will try reconnecting until connected. Setting `max_attempts` +* `max_attempts`: *0*; (Deprecated, please use `retry_strategy` instead) By default client will try reconnecting until connected. Setting `max_attempts` limits total amount of connection tries. Setting this to 1 will prevent any reconnect tries. * `retry_unfulfilled_commands`: *false*; If set to true, all commands that were unfulfulled while the connection is lost will be retried after the connection has reestablished again. Use this with caution, if you use state altering commands (e.g. *incr*). This is especially useful if you use blocking commands. -* `password`: *null*; If set, client will run redis auth command on connect. Alias `auth_pass` +* `password`: *null*; If set, client will run redis auth command on connect. Alias `auth_pass` (node_redis < 2.5 have to use auth_pass) * `db`: *null*; If set, client will run redis select command on connect. This is [not recommended](https://groups.google.com/forum/#!topic/redis-db/vS5wX8X4Cjg). * `family`: *IPv4*; You can force using IPv6 if you set the family to 'IPv6'. See Node.js [net](https://nodejs.org/api/net.html) or [dns](https://nodejs.org/api/dns.html) modules how to use the family type. * `disable_resubscribing`: *false*; If set to `true`, a client won't resubscribe after disconnecting diff --git a/index.js b/index.js index d5c0c01..8f755cc 100644 --- a/index.js +++ b/index.js @@ -51,20 +51,32 @@ function RedisClient (options) { for (var tls_option in options.tls) { // jshint ignore: line cnx_options[tls_option] = options.tls[tls_option]; } + // Warn on misusing deprecated functions + if (typeof options.retry_strategy === 'function') { + if ('max_attempts' in options) { + self.warn('WARNING: You activated the retry_strategy and max_attempts at the same time. This is not possible and max_attempts will be ignored.'); + // Do not print deprecation warnings twice + delete options.max_attempts; + } + if ('retry_max_delay' in options) { + self.warn('WARNING: You activated the retry_strategy and retry_max_delay at the same time. This is not possible and retry_max_delay will be ignored.'); + // Do not print deprecation warnings twice + delete options.retry_max_delay; + } + } + this.connection_options = cnx_options; - this.connection_id = ++connection_id; + this.connection_id = RedisClient.connection_id++; this.connected = false; this.ready = false; if (options.socket_nodelay === undefined) { options.socket_nodelay = true; } else if (!options.socket_nodelay) { // Only warn users with this set to false - process.nextTick(function () { - self.warn( - 'socket_nodelay is deprecated and will be removed in v.3.0.0.\n' + - 'Setting socket_nodelay to false likely results in a reduced throughput. Please use .batch to buffer commands and use pipelining.\n' + - 'If you are sure you rely on the NAGLE-algorithm you can activate it by calling client.stream.setNoDelay(false) instead.' - ); - }); + self.warn( + 'socket_nodelay is deprecated and will be removed in v.3.0.0.\n' + + 'Setting socket_nodelay to false likely results in a reduced throughput. Please use .batch for pipelining instead.\n' + + 'If you are sure you rely on the NAGLE-algorithm you can activate it by calling client.stream.setNoDelay(false) instead.' + ); } if (options.socket_keepalive === undefined) { options.socket_keepalive = true; @@ -76,9 +88,7 @@ function RedisClient (options) { options.detect_buffers = !!options.detect_buffers; // Override the detect_buffers setting if return_buffers is active and print a warning if (options.return_buffers && options.detect_buffers) { - process.nextTick(function () { - self.warn('WARNING: You activated return_buffers and detect_buffers at the same time. The return value is always going to be a buffer.'); - }); + self.warn('WARNING: You activated return_buffers and detect_buffers at the same time. The return value is always going to be a buffer.'); options.detect_buffers = false; } if (options.detect_buffers) { @@ -87,11 +97,27 @@ function RedisClient (options) { } this.should_buffer = false; this.max_attempts = options.max_attempts | 0; + if ('max_attempts' in options) { + self.warn( + 'max_attempts is deprecated and will be removed in v.3.0.0.\n' + + 'To reduce the amount of options and the improve the reconnection handling please use the new `retry_strategy` option instead.\n' + + 'This replaces the max_attempts and retry_max_delay option.' + ); + } this.command_queue = new Queue(); // Holds sent commands to de-pipeline them this.offline_queue = new Queue(); // Holds commands issued but not able to be sent + // ATTENTION: connect_timeout should change in v.3.0 so it does not count towards ending reconnection attempts after x seconds + // This should be done by the retry_strategy. Instead it should only be the timeout for connecting to redis this.connect_timeout = +options.connect_timeout || 3600000; // 60 * 60 * 1000 ms this.enable_offline_queue = options.enable_offline_queue === false ? false : true; this.retry_max_delay = +options.retry_max_delay || null; + if ('retry_max_delay' in options) { + self.warn( + 'retry_max_delay is deprecated and will be removed in v.3.0.0.\n' + + 'To reduce the amount of options and the improve the reconnection handling please use the new `retry_strategy` option instead.\n' + + 'This replaces the max_attempts and retry_max_delay option.' + ); + } this.initialize_retry_vars(); this.pub_sub_mode = false; this.subscription_set = {}; @@ -123,8 +149,24 @@ function RedisClient (options) { name: options.parser }); this.create_stream(); + // The listeners will not be attached right away, so let's print the deprecation message while the listener is attached + this.on('newListener', function (event) { + if (event === 'idle') { + this.warn( + 'The idle event listener is deprecated and will likely be removed in v.3.0.0.\n' + + 'If you rely on this feature please open a new ticket in node_redis with your use case' + ); + } else if (event === 'drain') { + this.warn( + 'The drain event listener is deprecated and will be removed in v.3.0.0.\n' + + 'If you want to keep on listening to this event please listen to the stream drain event directly.' + ); + } + }); } -util.inherits(RedisClient, events.EventEmitter); +util.inherits(RedisClient, EventEmitter); + +RedisClient.connection_id = 0; // Attention: the function name "create_stream" should not be changed, as other libraries need this to mock the stream (e.g. fakeredis) RedisClient.prototype.create_stream = function () { @@ -238,19 +280,23 @@ RedisClient.prototype.unref = function () { }; RedisClient.prototype.warn = function (msg) { - if (this.listeners('warning').length !== 0) { - this.emit('warning', msg); - } else { - console.warn('node_redis:', msg); - } + var self = this; + // Warn on the next tick. Otherwise no event listener can be added + // for warnings that are emitted in the redis client constructor + process.nextTick(function () { + if (self.listeners('warning').length !== 0) { + self.emit('warning', msg); + } else { + console.warn('node_redis:', msg); + } + }); }; // Flush provided queues, erroring any items with a callback first RedisClient.prototype.flush_and_error = function (error, queue_names) { - var command_obj; queue_names = queue_names || ['offline_queue', 'command_queue']; for (var i = 0; i < queue_names.length; i++) { - while (command_obj = this[queue_names[i]].shift()) { + for (var command_obj = this[queue_names[i]].shift(); command_obj; command_obj = this[queue_names[i]].shift()) { if (typeof command_obj.callback === 'function') { error.command = command_obj.command.toUpperCase(); command_obj.callback(error); @@ -419,9 +465,7 @@ RedisClient.prototype.ready_check = function () { }; RedisClient.prototype.send_offline_queue = function () { - var command_obj; - - while (command_obj = this.offline_queue.shift()) { + for (var command_obj = this.offline_queue.shift(); command_obj; command_obj = this.offline_queue.shift()) { debug('Sending offline command: ' + command_obj.command); this.send_command(command_obj.command, command_obj.args, command_obj.callback); } @@ -805,8 +849,7 @@ RedisClient.prototype.writeDefault = RedisClient.prototype.writeStrings = functi }; RedisClient.prototype.writeBuffers = function (data) { - var command; - while (command = this.pipeline_queue.shift()) { + for (var command = this.pipeline_queue.shift(); command; command = this.pipeline_queue.shift()) { this.stream.write(command); } this.should_buffer = !this.stream.write(data); diff --git a/test/commands/blpop.spec.js b/test/commands/blpop.spec.js index fc022a9..4105dcd 100644 --- a/test/commands/blpop.spec.js +++ b/test/commands/blpop.spec.js @@ -4,6 +4,7 @@ var assert = require("assert"); var config = require("../lib/config"); var helper = require("../helper"); var redis = config.redis; +var intercept = require('intercept-stdout'); describe("The 'blpop' method", function () { @@ -23,7 +24,14 @@ describe("The 'blpop' method", function () { it('pops value immediately if list contains values', function (done) { bclient = redis.createClient.apply(redis.createClient, args); redis.debug_mode = true; + var text = ''; + var unhookIntercept = intercept(function(data) { + text += data; + return ''; + }); client.rpush("blocking list", "initial value", helper.isNumber(1)); + unhookIntercept(); + assert(/^Send 127\.0\.0\.1:6379 id [0-9]+: \*3\r\n\$5\r\nrpush\r\n\$13\r\nblocking list\r\n\$13\r\ninitial value\r\n\n$/.test(text)); redis.debug_mode = false; bclient.blpop("blocking list", 0, function (err, value) { assert.strictEqual(value[0], "blocking list"); diff --git a/test/commands/get.spec.js b/test/commands/get.spec.js index 522872e..7f1f0cd 100644 --- a/test/commands/get.spec.js +++ b/test/commands/get.spec.js @@ -68,7 +68,6 @@ describe("The 'get' method", function () { }); it("gets the value correctly", function (done) { - client.GET(key, redis.print); // Use the utility function to print the result client.GET(key, function (err, res) { helper.isString(value)(err, res); done(err); diff --git a/test/commands/getset.spec.js b/test/commands/getset.spec.js index 11465bf..b39b028 100644 --- a/test/commands/getset.spec.js +++ b/test/commands/getset.spec.js @@ -33,7 +33,6 @@ describe("The 'getset' method", function () { }); it("reports an error", function (done) { - client.GET(key, redis.print); // Use the utility function to print the error client.get(key, function (err, res) { assert(err.message.match(/The connection has already been closed/)); done(); diff --git a/test/connection.spec.js b/test/connection.spec.js index 507089c..262face 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -4,6 +4,7 @@ var assert = require("assert"); var config = require("./lib/config"); var helper = require('./helper'); var redis = config.redis; +var intercept = require('intercept-stdout'); describe("connection tests", function () { helper.allTests(function(parser, ip, args) { @@ -91,6 +92,7 @@ describe("connection tests", function () { client.on("reconnecting", function (params) { client.end(true); + assert.strictEqual(params.times_connected, 1); setTimeout(done, 100); }); }); diff --git a/test/lib/stunnel-process.js b/test/lib/stunnel-process.js index f20bc22..f75049d 100644 --- a/test/lib/stunnel-process.js +++ b/test/lib/stunnel-process.js @@ -2,11 +2,16 @@ // helper to start and stop the stunnel process. var spawn = require('child_process').spawn; -var EventEmitter = require('events').EventEmitter; +var EventEmitter = require('events'); var fs = require('fs'); var path = require('path'); var util = require('util'); +// Newer Node.js versions > 0.10 return the EventEmitter right away and using .EventEmitter was deprecated +if (typeof EventEmitter !== 'function') { + EventEmitter = EventEmitter.EventEmitter; +} + function once(cb) { var called = false; return function() { diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index ef5090c..527633e 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -384,9 +384,18 @@ describe("The node_redis client", function () { describe('idle', function () { it('emits idle as soon as there are no outstanding commands', function (done) { + var end = helper.callFuncAfter(done, 2); + client.on('warning', function (msg) { + assert.strictEqual( + msg, + 'The idle event listener is deprecated and will likely be removed in v.3.0.0.\n' + + 'If you rely on this feature please open a new ticket in node_redis with your use case' + ); + end(); + }); client.on('idle', function onIdle () { client.removeListener("idle", onIdle); - client.get('foo', helper.isString('bar', done)); + client.get('foo', helper.isString('bar', end)); }); client.set('foo', 'bar'); }); @@ -614,9 +623,17 @@ describe("The node_redis client", function () { parser: parser, no_ready_check: true }); - var end = helper.callFuncAfter(done, 2); + var end = helper.callFuncAfter(done, 3); client.set('foo', 'bar'); client.get('foo', end); + client.on('warning', function (msg) { + assert.strictEqual( + msg, + 'The drain event listener is deprecated and will be removed in v.3.0.0.\n' + + 'If you want to keep on listening to this event please listen to the stream drain event directly.' + ); + end(); + }); client.on('drain', function() { assert(client.offline_queue.length === 0); end(); @@ -673,6 +690,9 @@ describe("The node_redis client", function () { client.on('reconnecting', function(params) { i++; assert.equal(params.attempt, i); + assert.strictEqual(params.times_connected, 0); + assert(params.error instanceof Error); + assert(typeof params.total_retry_time === 'number'); assert.strictEqual(client.offline_queue.length, 2); }); @@ -706,10 +726,6 @@ describe("The node_redis client", function () { helper.killConnection(client); }); - client.on("reconnecting", function (params) { - assert.equal(client.command_queue.length, 15); - }); - client.on('error', function(err) { if (/uncertain state/.test(err.message)) { assert.equal(client.command_queue.length, 0); @@ -787,10 +803,6 @@ describe("The node_redis client", function () { helper.killConnection(client); }); - client.on("reconnecting", function (params) { - assert.equal(client.command_queue.length, 15); - }); - client.on('error', function(err) { if (err.code === 'UNCERTAIN_STATE') { assert.equal(client.command_queue.length, 0); diff --git a/test/utils.spec.js b/test/utils.spec.js index d75e035..c886648 100644 --- a/test/utils.spec.js +++ b/test/utils.spec.js @@ -3,6 +3,7 @@ var assert = require('assert'); var Queue = require('double-ended-queue'); var utils = require('../lib/utils'); +var intercept = require('intercept-stdout'); describe('utils.js', function () { @@ -40,6 +41,30 @@ describe('utils.js', function () { }); }); + describe('print helper', function () { + it('callback with reply', function () { + var text = ''; + var unhookIntercept = intercept(function(data) { + text += data; + return ''; + }); + utils.print(null, 'abc'); + unhookIntercept(); + assert.strictEqual(text, 'Reply: abc\n'); + }); + + it('callback with error', function () { + var text = ''; + var unhookIntercept = intercept(function(data) { + text += data; + return ''; + }); + utils.print(new Error('Wonderful exception')); + unhookIntercept(); + assert.strictEqual(text, 'Error: Wonderful exception\n'); + }); + }); + describe('reply_in_order', function () { var err_count = 0;