From dffa8a6aee4ec39679aee584a50da485807e8ae5 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 15 Jan 2017 12:53:09 +0100 Subject: [PATCH] Fix parser being reset in case (p)message_buffer is attached without the parser set to return buffers. This might result in corrupt data if the listener is attached while the parser holds partial data. --- changelog.md | 6 +++ index.js | 10 ++++- test/pubsub.spec.js | 94 +++++++++++++++++++++++++-------------------- 3 files changed, 67 insertions(+), 43 deletions(-) diff --git a/changelog.md b/changelog.md index add6d9e5ad..3b0e7b8cfc 100644 --- a/changelog.md +++ b/changelog.md @@ -1,6 +1,12 @@ Changelog ========= +## v.2.6.5 - 15 Jan, 2017 + +Bugfixes + +- Fixed parser reset if (p)message_buffer listener is attached + ## v.2.6.4 - 12 Jan, 2017 Bugfixes diff --git a/index.js b/index.js index f81519d601..1b094f90b6 100644 --- a/index.js +++ b/index.js @@ -171,10 +171,16 @@ function RedisClient (options, stream) { '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.' ); - } else if (event === 'message_buffer' || event === 'pmessage_buffer' || event === 'messageBuffer' || event === 'pmessageBuffer' && !this.buffers) { + } else if ((event === 'message_buffer' || event === 'pmessage_buffer' || event === 'messageBuffer' || event === 'pmessageBuffer') && !this.buffers && !this.message_buffers) { + if (this.reply_parser.name !== 'javascript') { + return this.warn( + 'You attached the ' + event + ' without the hiredis parser without the returnBuffers option set to true.\n' + + 'Please use the JavaScript parser or set the returnBuffers option to true to return buffers.' + ); + } + this.reply_parser.optionReturnBuffers = true; this.message_buffers = true; this.handle_reply = handle_detect_buffers_reply; - this.reply_parser = create_parser(this); } }); } diff --git a/test/pubsub.spec.js b/test/pubsub.spec.js index 74d92c96e3..94b0fc1880 100644 --- a/test/pubsub.spec.js +++ b/test/pubsub.spec.js @@ -508,52 +508,64 @@ describe('publish/subscribe', function () { }); it('allows to listen to pmessageBuffer and pmessage', function (done) { - var batch = sub.batch(); var end = helper.callFuncAfter(done, 6); - assert.strictEqual(sub.message_buffers, false); - batch.psubscribe('*'); - batch.subscribe('/foo'); - batch.unsubscribe('/foo'); - batch.unsubscribe(helper.isNull()); - batch.subscribe(['/foo'], helper.isString('/foo')); - batch.exec(); - assert.strictEqual(sub.shouldBuffer, false); - sub.on('pmessageBuffer', function (pattern, channel, message) { - assert.strictEqual(pattern.inspect(), new Buffer('*').inspect()); - assert.strictEqual(channel.inspect(), new Buffer('/foo').inspect()); - sub.quit(end); - }); - // Either message_buffers or buffers has to be true, but not both at the same time - assert.notStrictEqual(sub.message_buffers, sub.buffers); - sub.on('pmessage', function (pattern, channel, message) { - assert.strictEqual(pattern, '*'); - assert.strictEqual(channel, '/foo'); - assert.strictEqual(message, 'hello world'); - end(); - }); - sub.on('message', function (channel, message) { - assert.strictEqual(channel, '/foo'); - assert.strictEqual(message, 'hello world'); - end(); - }); - setTimeout(function () { - pub.pubsub('numsub', '/foo', function (err, res) { - // There's one subscriber to this channel - assert.deepEqual(res, ['/foo', 1]); + var data = Array(10000).join('äüs^öéÉÉ`e'); + sub.set('foo', data, function () { + sub.get('foo'); + sub.stream.once('data', function () { + assert.strictEqual(sub.message_buffers, false); + assert.strictEqual(sub.shouldBuffer, false); + sub.on('pmessageBuffer', function (pattern, channel, message) { + if (parser !== 'javascript' && typeof pattern === 'string') { + pattern = new Buffer(pattern); + channel = new Buffer(channel); + } + assert.strictEqual(pattern.inspect(), new Buffer('*').inspect()); + assert.strictEqual(channel.inspect(), new Buffer('/foo').inspect()); + sub.quit(end); + }); + if (parser === 'javascript') { + assert.notStrictEqual(sub.message_buffers, sub.buffers); + } + + }); + var batch = sub.batch(); + batch.psubscribe('*'); + batch.subscribe('/foo'); + batch.unsubscribe('/foo'); + batch.unsubscribe(helper.isNull()); + batch.subscribe(['/foo'], helper.isString('/foo')); + batch.exec(function () { + pub.pubsub('numsub', '/foo', function (err, res) { + // There's one subscriber to this channel + assert.deepEqual(res, ['/foo', 1]); + end(); + }); + pub.pubsub('channels', function (err, res) { + // There's exactly one channel that is listened too + assert.deepEqual(res, ['/foo']); + end(); + }); + pub.pubsub('numpat', function (err, res) { + // One pattern is active + assert.strictEqual(res, 1); + end(); + }); + pub.publish('/foo', 'hello world', helper.isNumber(2)); + }); + // Either message_buffers or buffers has to be true, but not both at the same time + sub.on('pmessage', function (pattern, channel, message) { + assert.strictEqual(pattern, '*'); + assert.strictEqual(channel, '/foo'); + assert.strictEqual(message, 'hello world'); end(); }); - pub.pubsub('channels', function (err, res) { - // There's exactly one channel that is listened too - assert.deepEqual(res, ['/foo']); + sub.on('message', function (channel, message) { + assert.strictEqual(channel, '/foo'); + assert.strictEqual(message, 'hello world'); end(); }); - pub.pubsub('numpat', function (err, res) { - // One pattern is active - assert.strictEqual(res, 1); - end(); - }); - pub.publish('/foo', 'hello world', helper.isNumber(2)); - }, 50); + }); }); });