Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Javascript parser swallows errors thrown in command callback #361

Closed
wants to merge 2 commits into from

Conversation

dainis
Copy link

@dainis dainis commented Jan 4, 2013

Looks like error is here : https://github.com/mranney/node_redis/blob/master/lib/parser/javascript.js#L213

This try/catch catches also errors thrown from callbacks, but error isn't thrown/emited to client

in case of pubsub message same message will go on for ever

var redis = require('redis'),
    r = redis.createClient(3052, '127.0.0.1'),
    r2 = redis.createClient(3052, '127.0.0.1');

r.on('ready', function() {
    r.subscribe('channel');

    r.on('error', function(e){
        console.log(e);
    });

    r.on('message', function(channel, data) {
        console.log(channel, data);
        throw new Error('test');
    });
});

r2.on('ready', function() {
    var i = 0;
    console.log('r2');
    (function p() {
        r2.publish('channel', ++i);
        setTimeout(p, 1000);
    })()
});

output:

channel 2
channel 2
channel 2
...

Ready to merge checklist

  • test(s) in test.js
  • tests will fail without the PR, but succeed once applied
  • docs, if applicable
  • merges cleanly
  • coding style (4-space indents, looks similar to other code)

dainis and others added 2 commits January 4, 2013 22:49
…ected to be from parsing process, but if error is thrown in callback then it is handled in the same way, meaning that it never bubbles up to the actual application (issue redis#361)
An uncaught exception will be raised when the retry timer tries to
reconnect and encounter an error, for all event listeners of the stream
were removed in line 826. It should set closing varable to be true.
@cxreg
Copy link

cxreg commented Jan 5, 2013

Note, this addresses the same issue as #357.

@brycebaril
Copy link
Contributor

I think this is on the right track, though there is another Error the parser can through on incomplete read frames, the line https://github.com/dainis/node_redis/blob/master/lib/parser/javascript.js#L276

            throw new Error("didn't see LF after NL reading multi bulk count (" + offset + " => " + this._buffer.length + ", " + this._offset + ")");

I'm pretty sure that is another flavor of the 'too far' type Error here.

In terms of naming, given that these 'too far' Errors are fairly expected, maybe name the Exception something like IncompleteReadBuffer or something that better indicates what is going on. (I.e. the TCP packet was broken up in the middle of the reply it is parsing, so it needs the next packet to append to the buffer before it can continue)

This is definitely a very large issue with the current parser and one of the first things we'd like to address in the next version of node_redis.

@brycebaril
Copy link
Contributor

This work was done as part of #382

@brycebaril brycebaril closed this Mar 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants