Skip to content

Commit a857829

Browse files
author
Ruben Bridgewater
committed
Improve error handling
Arguments are now passed to an command error in case they exist An error is only emitted if that very same error is not already handled in a callback
1 parent 97ae788 commit a857829

File tree

8 files changed

+103
-46
lines changed

8 files changed

+103
-46
lines changed

‎index.js‎

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ var tls = require('tls');
55
var util = require('util');
66
var utils = require('./lib/utils');
77
var Queue = require('double-ended-queue');
8+
var CommandError = require('./lib/customError');
89
var Command = require('./lib/command').Command;
910
var OfflineCommand = require('./lib/command').OfflineCommand;
1011
var EventEmitter = require('events');
@@ -264,11 +265,11 @@ RedisClient.prototype.create_stream = function () {
264265
});
265266

266267
this.stream.once('close', function (hadError) {
267-
self.connection_gone('close', new Error('Stream connection closed' + (hadError ? ' because of a transmission error' : '')));
268+
self.connection_gone('close', hadError ? new Error('Stream connection closed with a transmission error') : null);
268269
});
269270

270271
this.stream.once('end', function () {
271-
self.connection_gone('end', new Error('Stream connection ended'));
272+
self.connection_gone('end', null);
272273
});
273274

274275
this.stream.on('drain', function () {
@@ -320,16 +321,29 @@ RedisClient.prototype.warn = function (msg) {
320321

321322
// Flush provided queues, erroring any items with a callback first
322323
RedisClient.prototype.flush_and_error = function (error, queue_names) {
324+
var callbacks_not_called = [];
323325
queue_names = queue_names || ['offline_queue', 'command_queue'];
324326
for (var i = 0; i < queue_names.length; i++) {
325327
for (var command_obj = this[queue_names[i]].shift(); command_obj; command_obj = this[queue_names[i]].shift()) {
328+
var err = new CommandError(error);
329+
err.command = command_obj.command.toUpperCase();
330+
if (command_obj.args.length) {
331+
err.args = command_obj.args;
332+
}
326333
if (typeof command_obj.callback === 'function') {
327-
error.command = command_obj.command.toUpperCase();
328-
command_obj.callback(error);
334+
command_obj.callback(err);
335+
} else {
336+
callbacks_not_called.push(err);
329337
}
330338
}
331339
this[queue_names[i]] = new Queue();
332340
}
341+
// Mutate the original error that will be emitted
342+
// This is fine, as we don't manipulate any user errors
343+
if (callbacks_not_called.length !== 0) {
344+
error.errors = callbacks_not_called;
345+
}
346+
return callbacks_not_called.length === 0;
333347
};
334348

335349
RedisClient.prototype.on_error = function (err) {
@@ -546,8 +560,10 @@ RedisClient.prototype.connection_gone = function (why, error) {
546560

547561
// If this is a requested shutdown, then don't retry
548562
if (this.closing) {
549-
debug('Connection ended from quit command, not retrying.');
550-
this.flush_and_error(new Error('Redis connection gone from ' + why + ' event.'));
563+
debug('Connection ended by quit / end command, not retrying.');
564+
error = new Error('Stream connection ended and running command aborted. It might have been processed.');
565+
error.code = 'NR_OFFLINE';
566+
this.flush_and_error(error);
551567
return;
552568
}
553569

@@ -567,10 +583,18 @@ RedisClient.prototype.connection_gone = function (why, error) {
567583
if (typeof this.retry_delay !== 'number') {
568584
// Pass individual error through
569585
if (this.retry_delay instanceof Error) {
570-
error = this.retry_delay;
586+
error = new CommandError(this.retry_delay);
587+
}
588+
// Attention: there might be the case where there's no error!
589+
if (!error) {
590+
error = new Error('Stream connection ended and running command aborted. It might have been processed.');
591+
error.code = 'NR_OFFLINE';
592+
}
593+
// Only emit an error in case that a running command had no callback
594+
if (!this.flush_and_error(error)) {
595+
error.message = 'Stream connection ended and all running commands aborted. They might have been processed.';
596+
this.emit('error', error);
571597
}
572-
this.flush_and_error(error);
573-
this.emit('error', error);
574598
this.end(false);
575599
return;
576600
}
@@ -595,11 +619,11 @@ RedisClient.prototype.connection_gone = function (why, error) {
595619
} else if (this.command_queue.length !== 0) {
596620
error = new Error('Redis connection lost and command aborted in uncertain state. It might have been processed.');
597621
error.code = 'UNCERTAIN_STATE';
598-
this.flush_and_error(error, ['command_queue']);
599-
error.message = 'Redis connection lost and commands aborted in uncertain state. They might have been processed.';
600-
// TODO: Reconsider emitting this always, as each running command is handled anyway
601-
// This should likely be removed in v.3. This is different to the broken connection as we'll reconnect here
602-
this.emit('error', error);
622+
if (!this.flush_and_error(error, ['command_queue'])) {
623+
// Only emit if not all commands had a callback that already handled the error
624+
error.message = 'Redis connection lost and commands aborted in uncertain state. They might have been processed.';
625+
this.emit('error', error);
626+
}
603627
}
604628

605629
if (this.retry_max_delay !== null && this.retry_delay > this.retry_max_delay) {
@@ -618,6 +642,9 @@ RedisClient.prototype.return_error = function (err) {
618642
var command_obj = this.command_queue.shift();
619643
if (command_obj && command_obj.command && command_obj.command.toUpperCase) {
620644
err.command = command_obj.command.toUpperCase();
645+
if (command_obj.args.length) {
646+
err.args = command_obj.args;
647+
}
621648
}
622649

623650
var match = err.message.match(utils.err_code);
@@ -786,6 +813,9 @@ function handle_offline_command (self, command_obj) {
786813
}
787814
err = new Error(command + " can't be processed. " + msg);
788815
err.command = command;
816+
if (command_obj.args.length) {
817+
err.args = command_obj.args;
818+
}
789819
err.code = 'NR_OFFLINE';
790820
utils.reply_in_order(self, callback, err);
791821
} else {

‎lib/customError.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
'use strict';
2+
3+
var util = require('util');
4+
5+
function CommandError (error) {
6+
Error.captureStackTrace(this, this.constructor);
7+
this.name = this.constructor.name;
8+
this.message = error.message;
9+
for (var keys = Object.keys(error), key = keys.pop(); key; key = keys.pop()) {
10+
this[key] = error[key];
11+
}
12+
}
13+
14+
util.inherits(CommandError, Error);
15+
16+
module.exports = CommandError;

‎lib/extendedApi.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,10 @@ RedisClient.prototype.send_command = RedisClient.prototype.sendCommand = functio
4747
RedisClient.prototype.end = function (flush) {
4848
// Flush queue if wanted
4949
if (flush) {
50-
this.flush_and_error(new Error("The command can't be processed. The connection has already been closed."));
50+
var err = new Error("The command can't be processed. The connection has already been closed.");
51+
err.code = 'NR_OFFLINE';
52+
this.flush_and_error(err);
53+
// TODO: Emit an error in case a command did not have a callback
5154
} else if (arguments.length === 0) {
5255
this.warn(
5356
'Using .end() without the flush parameter is deprecated and throws from v.3.0.0 on.\n' +

‎lib/multi.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ function Multi (client, args) {
2323
function pipeline_transaction_command (self, command, args, index, cb, call_on_write) {
2424
// Queueing is done first, then the commands are executed
2525
self._client.send_command(command, args, function (err, reply) {
26-
if (err) {
26+
// Ignore the multi command. This is applied by node_redis and the user does not benefit by it
27+
if (err && index !== -1) {
2728
if (cb) {
2829
cb(err);
2930
}
@@ -44,13 +45,12 @@ function multi_callback (self, err, replies) {
4445
var i = 0, args;
4546

4647
if (err) {
47-
// The errors would be circular
48-
var connection_error = ['CONNECTION_BROKEN', 'UNCERTAIN_STATE'].indexOf(err.code) !== -1;
49-
err.errors = connection_error ? [] : self.errors;
48+
err.errors = self.errors;
49+
err.command = 'EXEC';
5050
if (self.callback) {
5151
self.callback(err);
5252
// Exclude connection errors so that those errors won't be emitted twice
53-
} else if (!connection_error) {
53+
} else if (err.code !== 'CONNECTION_BROKEN') {
5454
self._client.emit('error', err);
5555
}
5656
return;
@@ -91,7 +91,7 @@ Multi.prototype.exec_transaction = function exec_transaction (callback) {
9191
self.callback = callback;
9292
self._client.cork();
9393
self.wants_buffers = new Array(len);
94-
pipeline_transaction_command(self, 'multi', []);
94+
pipeline_transaction_command(self, 'multi', [], -1);
9595
// Drain queue, callback will catch 'QUEUED' or error
9696
for (var index = 0; index < len; index++) {
9797
// The commands may not be shifted off, since they are needed in the result handler

‎test/auth.spec.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ describe('client authentication', function () {
183183
}
184184
});
185185
client.on('reconnecting', function (params) {
186-
assert.strictEqual(params.error.message, 'Stream connection closed');
186+
assert.strictEqual(params.error, null);
187187
});
188188
});
189189

‎test/connection.spec.js‎

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ describe('connection tests', function () {
5353
}
5454
});
5555
client.set('foo', 'bar', function (err, res) {
56-
assert.strictEqual(err.message, 'Redis connection gone from close event.');
56+
assert.strictEqual(err.message, 'Stream connection ended and running command aborted. It might have been processed.');
5757
called = -1;
5858
});
5959
});
@@ -62,7 +62,7 @@ describe('connection tests', function () {
6262
var called = false;
6363
client = redis.createClient(9999);
6464
client.set('foo', 'bar', function (err, res) {
65-
assert.strictEqual(err.message, 'Redis connection gone from close event.');
65+
assert.strictEqual(err.message, 'Stream connection ended and running command aborted. It might have been processed.');
6666
called = true;
6767
});
6868
var bool = client.quit(function (err, res) {
@@ -277,13 +277,12 @@ describe('connection tests', function () {
277277
text += data;
278278
return '';
279279
});
280-
var end = helper.callFuncAfter(done, 2);
281280
client = redis.createClient({
282281
retryStrategy: function (options) {
283282
if (options.totalRetryTime > 150) {
284283
client.set('foo', 'bar', function (err, res) {
285284
assert.strictEqual(err.message, 'Connection timeout');
286-
end();
285+
done();
287286
});
288287
// Pass a individual error message to the error handler
289288
return new Error('Connection timeout');
@@ -294,40 +293,30 @@ describe('connection tests', function () {
294293
retryMaxDelay: 123,
295294
port: 9999
296295
});
297-
298-
client.on('error', function (err) {
299-
unhookIntercept();
296+
process.nextTick(function () {
300297
assert.strictEqual(
301298
text,
302299
'node_redis: WARNING: You activated the retry_strategy and max_attempts at the same time. This is not possible and max_attempts will be ignored.\n' +
303300
'node_redis: 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.\n'
304301
);
305-
assert.strictEqual(err.message, 'Connection timeout');
306-
assert(!err.code);
307-
end();
302+
unhookIntercept();
308303
});
309304
});
310305

311306
it('retry_strategy used to reconnect', function (done) {
312-
var end = helper.callFuncAfter(done, 2);
313307
client = redis.createClient({
314308
retry_strategy: function (options) {
315309
if (options.total_retry_time > 150) {
316310
client.set('foo', 'bar', function (err, res) {
317311
assert.strictEqual(err.code, 'ECONNREFUSED');
318-
end();
312+
done();
319313
});
320314
return false;
321315
}
322316
return Math.min(options.attempt * 25, 200);
323317
},
324318
port: 9999
325319
});
326-
327-
client.on('error', function (err) {
328-
assert.strictEqual(err.code, 'ECONNREFUSED');
329-
end();
330-
});
331320
});
332321
});
333322

‎test/multi.spec.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,8 @@ describe("The 'multi' method", function () {
180180

181181
client.multi([['set', 'foo', 'bar'], ['get', 'foo']]).exec(function (err, res) {
182182
assert(/Redis connection in broken state/.test(err.message));
183-
assert.strictEqual(err.errors.length, 0);
183+
assert.strictEqual(err.errors.length, 2);
184+
assert.strictEqual(err.errors[0].args.length, 2);
184185
});
185186
});
186187

‎test/node_redis.spec.js‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,7 @@ describe('The node_redis client', function () {
366366
client = redis.createClient();
367367
client.quit(function () {
368368
client.get('foo', function (err, res) {
369-
assert(err.message.indexOf('Redis connection gone') !== -1);
369+
assert.strictEqual(err.message, 'Stream connection ended and running command aborted. It might have been processed.');
370370
assert.strictEqual(client.offline_queue.length, 0);
371371
done();
372372
});
@@ -1024,14 +1024,25 @@ describe('The node_redis client', function () {
10241024
helper.killConnection(client);
10251025
});
10261026

1027+
var end = helper.callFuncAfter(done, 3);
10271028
client.on('error', function (err) {
1028-
if (/uncertain state/.test(err.message)) {
1029-
assert.equal(client.command_queue.length, 0);
1030-
done();
1029+
if (err.command === 'EXEC') {
1030+
assert.strictEqual(client.command_queue.length, 0);
1031+
assert.strictEqual(err.errors.length, 9);
1032+
assert.strictEqual(err.errors[1].command, 'SET');
1033+
assert.deepEqual(err.errors[1].args, ['foo1', 'bar1']);
1034+
end();
1035+
} else if (err.code === 'UNCERTAIN_STATE') {
1036+
assert.strictEqual(client.command_queue.length, 0);
1037+
assert.strictEqual(err.errors.length, 4);
1038+
assert.strictEqual(err.errors[0].command, 'SET');
1039+
assert.deepEqual(err.errors[0].args, ['foo0', 'bar0']);
1040+
end();
10311041
} else {
10321042
assert.equal(err.code, 'ECONNREFUSED');
10331043
assert.equal(err.errno, 'ECONNREFUSED');
10341044
assert.equal(err.syscall, 'connect');
1045+
end();
10351046
}
10361047
});
10371048
});
@@ -1101,14 +1112,21 @@ describe('The node_redis client', function () {
11011112
helper.killConnection(client);
11021113
});
11031114

1115+
var end = helper.callFuncAfter(done, 3);
11041116
client.on('error', function (err) {
1105-
if (err.code === 'UNCERTAIN_STATE') {
1117+
if (err.command === 'EXEC') {
11061118
assert.equal(client.command_queue.length, 0);
1107-
done();
1119+
assert.equal(err.errors.length, 9);
1120+
end();
1121+
} else if (err.code === 'UNCERTAIN_STATE') {
1122+
assert.equal(client.command_queue.length, 0);
1123+
assert.equal(err.errors.length, 4);
1124+
end();
11081125
} else {
11091126
assert.equal(err.code, 'ECONNREFUSED');
11101127
assert.equal(err.errno, 'ECONNREFUSED');
11111128
assert.equal(err.syscall, 'connect');
1129+
end();
11121130
}
11131131
});
11141132
});

0 commit comments

Comments
 (0)