Skip to content

Commit 7e68925

Browse files
BridgeARBridgeAR
committed
Merge pull request #852 from fintura/parser-fix
Don't catch callback errors with the parser; fix a parser regression; remove dead code
2 parents feb528a + 083e446 commit 7e68925

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

50 files changed

+80
-130
lines changed

‎index.js

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,9 @@ RedisClient.prototype.install_stream_listeners = function() {
8686
});
8787

8888
this.stream.on("data", function (buffer_from_socket) {
89-
self.on_data(buffer_from_socket);
89+
// The data.toString() has a significant impact on big chunks and therefor this should only be used if necessary
90+
// debug("Net read " + this.address + " id " + this.connection_id + ": " + data.toString());
91+
self.reply_parser.execute(buffer_from_socket);
9092
});
9193

9294
this.stream.on("error", function (msg) {
@@ -273,8 +275,18 @@ RedisClient.prototype.init_parser = function () {
273275
this.reply_parser = new this.parser_module.Parser({
274276
return_buffers: self.options.return_buffers || self.options.detect_buffers || false
275277
});
276-
this.reply_parser.send_error = this.return_error.bind(self);
277-
this.reply_parser.send_reply = this.return_reply.bind(self);
278+
// Important: Only send results / errors async.
279+
// That way the result / error won't stay in a try catch block and catch user things
280+
this.reply_parser.send_error = function (data) {
281+
process.nextTick(function() {
282+
this.return_error(data);
283+
}.bind(this));
284+
}.bind(this);
285+
this.reply_parser.send_reply = function (data) {
286+
process.nextTick(function() {
287+
this.return_reply(data);
288+
}.bind(this));
289+
}.bind(this);
278290
};
279291

280292
RedisClient.prototype.on_ready = function () {
@@ -488,33 +500,22 @@ RedisClient.prototype.connection_gone = function (why) {
488500
this.retry_timer = setTimeout(retry_connection, this.retry_delay, this);
489501
};
490502

491-
RedisClient.prototype.on_data = function (data) {
492-
// The data.toString() has a significant impact on big chunks and therefor this should only be used if necessary
493-
// debug("Net read " + this.address + " id " + this.connection_id + ": " + data.toString());
494-
495-
try {
496-
this.reply_parser.execute(data);
497-
} catch (err) {
498-
// This is an unexpected parser problem, an exception that came from the parser code itself.
499-
// Parser should emit "error" events if it notices things are out of whack.
500-
// Callbacks that throw exceptions will land in return_reply(), below.
501-
// TODO - it might be nice to have a different "error" event for different types of errors
502-
this.emit("error", err);
503-
}
504-
};
505-
506503
RedisClient.prototype.return_error = function (err) {
507504
var command_obj = this.command_queue.shift(), queue_len = this.command_queue.length;
508-
err.command_used = command_obj.command.toUpperCase();
505+
if (command_obj.command && command_obj.command.toUpperCase) {
506+
err.command_used = command_obj.command.toUpperCase();
507+
}
509508

510509
if (this.pub_sub_mode === false && queue_len === 0) {
511510
this.command_queue = new Queue();
512511
this.emit("idle");
513512
}
513+
514514
if (this.should_buffer && queue_len <= this.command_queue_low_water) {
515515
this.emit("drain");
516516
this.should_buffer = false;
517517
}
518+
518519
if (command_obj.callback) {
519520
command_obj.callback(err);
520521
} else {

‎lib/parser/javascript.js

Lines changed: 14 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -55,19 +55,13 @@ ReplyParser.prototype._parseResult = function (type) {
5555
}
5656

5757
if (type === 45) {
58-
var result = this._buffer.toString(this._encoding, start, end);
59-
return new Error(result);
60-
}
61-
62-
if (this.options.return_buffers) {
58+
return new Error(this._buffer.toString(this._encoding, start, end));
59+
} else if (this.options.return_buffers) {
6360
return this._buffer.slice(start, end);
64-
} else {
65-
if (end - start < 65536) { // completely arbitrary
66-
return small_toString(this._buffer, start, end);
67-
} else {
68-
return this._buffer.toString(this._encoding, start, end);
69-
}
61+
} else if (end - start < 65536) { // completely arbitrary
62+
return small_toString(this._buffer, start, end);
7063
}
64+
return this._buffer.toString(this._encoding, start, end);
7165
} else if (type === 58) { // :
7266
// up to the delimiter
7367
end = this._packetEndOffset() - 1;
@@ -92,7 +86,7 @@ ReplyParser.prototype._parseResult = function (type) {
9286

9387
// packets with a size of -1 are considered null
9488
if (packetHeader.size === -1) {
95-
return undefined;
89+
return null;
9690
}
9791

9892
end = this._offset + packetHeader.size;
@@ -108,10 +102,9 @@ ReplyParser.prototype._parseResult = function (type) {
108102

109103
if (this.options.return_buffers) {
110104
return this._buffer.slice(start, end);
111-
} else {
112-
return this._buffer.toString(this._encoding, start, end);
113105
}
114-
} else if (type === 42) { // *
106+
return this._buffer.toString(this._encoding, start, end);
107+
} else { // *
115108
offset = this._offset;
116109
packetHeader = new Packet(type, this.parseHeader());
117110

@@ -136,9 +129,6 @@ ReplyParser.prototype._parseResult = function (type) {
136129
throw new IncompleteReadBuffer("Wait for more data.");
137130
}
138131
res = this._parseResult(ntype);
139-
if (res === undefined) {
140-
res = null;
141-
}
142132
reply.push(res);
143133
}
144134

@@ -176,18 +166,8 @@ ReplyParser.prototype.execute = function (buffer) {
176166
} else if (type === 36) { // $
177167
ret = this._parseResult(type);
178168

179-
if (ret === null) {
180-
break;
181-
}
182-
183-
// check the state for what is the result of
184-
// a -1, set it back up for a null reply
185-
if (ret === undefined) {
186-
ret = null;
187-
}
188-
189169
this.send_reply(ret);
190-
} else if (type === 42) { // *
170+
} else if (type === 42) { // 42 *
191171
// set a rewind point. if a failure occurs,
192172
// wait for the next execute()/append() and try again
193173
offset = this._offset - 1;
@@ -199,9 +179,6 @@ ReplyParser.prototype.execute = function (buffer) {
199179
} catch (err) {
200180
// catch the error (not enough data), rewind, and wait
201181
// for the next packet to appear
202-
if (! (err instanceof IncompleteReadBuffer)) {
203-
throw err;
204-
}
205182
this._offset = offset;
206183
break;
207184
}
@@ -243,6 +220,11 @@ ReplyParser.prototype._packetEndOffset = function () {
243220

244221
while (this._buffer[offset] !== 0x0d && this._buffer[offset + 1] !== 0x0a) {
245222
offset++;
223+
224+
/* istanbul ignore if: activate the js parser out of memory test to test this */
225+
if (offset >= this._buffer.length) {
226+
throw new IncompleteReadBuffer("didn't see LF after NL reading multi bulk count (" + offset + " => " + this._buffer.length + ", " + this._offset + ")");
227+
}
246228
}
247229

248230
offset++;

‎test/commands/blpop.spec.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ describe("The 'blpop' method", function () {
1515

1616
beforeEach(function (done) {
1717
client = redis.createClient.apply(redis.createClient, args);
18-
client.once("error", done);
1918
client.once("connect", function () {
2019
client.flushdb(done);
2120
});

‎test/commands/client.spec.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ describe("The 'client' method", function () {
1515

1616
beforeEach(function (done) {
1717
client = redis.createClient.apply(redis.createClient, args);
18-
client.once("error", done);
1918
client.once("connect", function () {
2019
client.flushdb(done);
2120
});

‎test/commands/dbsize.spec.js

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ describe("The 'dbsize' method", function () {
2323

2424
beforeEach(function (done) {
2525
client = redis.createClient.apply(redis.createClient, args);
26-
client.once("error", done);
2726
client.once("connect", function () {
2827
client.quit();
2928
});
@@ -45,7 +44,6 @@ describe("The 'dbsize' method", function () {
4544

4645
beforeEach(function (done) {
4746
client = redis.createClient.apply(redis.createClient, args);
48-
client.once("error", done);
4947
client.once("connect", function () {
5048
client.flushdb(function (err, res) {
5149
helper.isString("OK")(err, res);

‎test/commands/del.spec.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ describe("The 'del' method", function () {
1313

1414
beforeEach(function (done) {
1515
client = redis.createClient.apply(redis.createClient, args);
16-
client.once("error", done);
1716
client.once("connect", function () {
1817
client.flushdb(done);
1918
});

‎test/commands/eval.spec.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ describe("The 'eval' method", function () {
1616

1717
beforeEach(function (done) {
1818
client = redis.createClient.apply(redis.createClient, args);
19-
client.once("error", done);
2019
client.once("connect", function () {
2120
client.flushdb(done);
2221
});

‎test/commands/exits.spec.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ describe("The 'exits' method", function () {
1313

1414
beforeEach(function (done) {
1515
client = redis.createClient.apply(redis.createClient, args);
16-
client.once("error", done);
1716
client.once("connect", function () {
1817
client.flushdb(done);
1918
});

‎test/commands/expire.spec.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ describe("The 'expire' method", function () {
1313

1414
beforeEach(function (done) {
1515
client = redis.createClient.apply(redis.createClient, args);
16-
client.once("error", done);
1716
client.once("connect", function () {
1817
client.flushdb(done);
1918
});

‎test/commands/flushdb.spec.js

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ describe("The 'flushdb' method", function () {
2323

2424
beforeEach(function (done) {
2525
client = redis.createClient.apply(redis.createClient, args);
26-
client.once("error", done);
2726
client.once("connect", function () {
2827
client.quit();
2928
});
@@ -45,7 +44,6 @@ describe("The 'flushdb' method", function () {
4544

4645
beforeEach(function (done) {
4746
client = redis.createClient.apply(redis.createClient, args);
48-
client.once("error", done);
4947
client.once("connect", function () {
5048
done();
5149
});

0 commit comments

Comments
 (0)