MULTI EXEC will not return error when some of the commands fails

[bitinn]

It took me a long time to figure out why: when some of queued commands fail to execute, Redis server send reply as Bulk Strings, and in both node_redis javascript parser and hiredis wrapper, they are emitted as reply, not reply error.

Thus in exec callback, err will be null, res will be an array containing all replies, both ok and error. eg.

[ 'OK',
  'WRONGTYPE Operation against a key holding the wrong kind of value',
  'WRONGTYPE Operation against a key holding the wrong kind of value' ]

(note the errors are returned as string, not wrapped in Error, so no easy way to determine whether MULTI EXEC are run successfully)

I am not sure the best way to fix this, changing how node_redis parser handle this type of responses will probably cause it to diverge with hiredis behaviour...

But at the very least we should document it, so issues don't come up again and again, like #624 & #550.

What do you guys think? @brycebaril @mranney @mocheng @bsnote @guileen (apologize for the mass notice if you are not interested)

Reference:

• http://redis.io/topics/transactions - see the end of Errors inside a transaction section, where it explicitly states, It's up to the client library to find a sensible way to provide the error to the user.
• http://redis.io/topics/protocol#bulk-string-reply
• https://github.com/mranney/node_redis/blob/master/lib/parser/javascript.js#L190

[brycebaril]

@bitinn good find and research on this!

In terms of diverging on the hiredis parser, most likely we'd need to pair this with a pull request for that library as well. I'm not as concerned about that for a few reasons: 1. the javascript parser is better in most cases these days, and 2. it won't even work with Node 0.11 or 0.12 without its own major work.

As for fixing this library going forward (in the case where we disregard hiredis for now) I think the first step is to create a canonical set of test cases and output so we can decide we have a sufficient mechanism for deducing these errors. Hopefully that's not going through Redis source to collect all the error messages, but it may come to that.

[bitinn]

@brycebaril I don't fully understand why redis-server can't reply like:

*2\r\n
+OK\r\n
-Error\r\n

but instead of use

*2\r\n
$2\r\n
OK\r\n
$5\r\n
Error\r\n

At least that's what the protocol document are suggesting?

UPDATE: ah, because some commands doesn't reply with simple string, silly me... hopefully redis-server stick to a certain way of encoding responses as bulk strings, so we can use a subroutine to parse and decode them.

[Guuz]

I ran into the same problem and was about to file an issue when i saw this one.
Any tips on how to code around this? I'm not sure how to do my error handling now and how to assure the command(s) executed successfully.

Semi-related:
If you did actually get an array of errors, what should i do with them? My higher function would probably expect an error object, not an array of errors. This is not standard.

[brycebaril]

For now the best advice I can offer is don't have this problem. It's lame, I know, but one nice thing about Redis is the only errors are operations that should be avoidable, e.g. bad command syntax or mismatching commands and types. Any time you do get an Error from Redis should likely be considered a bug in your use of Redis, and an invalid application.

Of course that isn't the long-term solution because we need our applications to be robust in the face of uncertainty. Beyond that it's going to require working around this quirk in the Redis protocol and inspecting the replies.

[Guuz]

Thanks for the reply @brycebaril! I understand what you are saying, that most errors are developer errors. The case I was specifically looking at was a disconnect. If our Redis goes down 500 server errors are to be expected (we have a reconnect in place) but the errors should be clear/logged/tracable. If it's an array of errors this might not be the case, nor if the error is not exposed as an error but a normal result string...

I hope it will be rare but because an error case is so rare you want to have the right logging/information when it happens.

[mbfisher]

Found some useful docs on this which confirm it is expected behaviour, might help others:

Why Redis does not support roll backs?

If you have a relational databases background, the fact that Redis commands can fail during a transaction, but still Redis will execute the rest of the transaction instead of rolling back, may look odd to you.
However there are good opinions for this behavior:

• Redis commands can fail only if called with a wrong syntax (and the problem is not detectable during the command queueing), or against keys holding the wrong data type: this means that in practical terms a failing command is the result of a programming errors, and a kind of error that is very likely to be detected during development, and not in production.
• Redis is internally simplified and faster because it does not need the ability to roll back.

An argument against Redis point of view is that bugs happen, however it should be noted that in general the roll back does not save you from programming errors. For instance if a query increments a key by 2 instead of 1, or increments the wrong key, there is no way for a rollback mechanism to help. Given that no one can save the programmer from his errors, and that the kind of errors required for a Redis command to fail are unlikely to enter in production, we selected the simpler and faster approach of not supporting roll backs on errors.

M

[blainsmith]

In order to better support this project and its new group of collaborators under a new org we are trying to clean up and close issues older than 6 months. Your issue may have been fixed since the issue was open, however, if you are still experiencing a problem please re-open this issue and we will label it accordingly.

[bitinn]

@blainsmith It's a known issue for redis clients, please look into it when available. I would love to see how you handle this in general.

EXEC returned two-element Bulk string reply where one is an OK code and the other an -ERR reply. It's up to the client library to find a sensible way to provide the error to the user.

[simontabor]

@bitinn What would you say the client should do? Some thoughts:

• The multi hasn't failed, so handling a single command error by immediately giving an err to the exec callback would be misleading (all the other commands could have succeeded)
• Individual commands can still have their own error handlers.

In https://github.com/gosquared/redis-clustr, I handle this by keeping an array of errors and another of responses and simply callback with both. For example:

var multi = redis.multi();
multi.get('test', function(err, value) { }); // pretend this errors
multi.get('test2', function(err, value) { }); // and this succeeds
multi.exec(function(errs, resp) {
  // errs = [ Error ]
  // resp = [ undefined, value ]
});

This isn't a great way of handling it (you can't see what commands errored easily), and individual command callbacks are much better.

My personal opinion is that errors within the exec callback should be Errors (not strings) and we should recommend handling errors on a command-by-command basis.

var multi = redis.multi();
multi.get('test', function(err, value) { /* errors should be handled here */ });
multi.get('test2', function(err, value) { /* ... */ });
multi.exec(function(err, resp) {
  // err = null
  // resp = [ Error || value, Error || value ... ]
});

Not got terribly strong opinions on this though so would love to hear ideas!

[bitinn]

I think I prefer introducing a new error type, say TransactionError, so that err in exec callback can be handled by if(err) { ... } AND provide custom data for error handling.

I haven't looked into this since, so I am not sure whether this is possible with current node_redis parser.

[bcoe]

Looping in @dohse, who has submitted a partial solution to this problem.

#751

I like the simplicity of this solution, but I agree with concerns that it's a bit strange that errors and good responses are intermixed.

This might be the best solution in the short term, however, since I think switching to returning an error in the errback would require a major semver bump?

[BridgeAR]

@bitinn does this apply to the the js parser and the hiredis parser or only to the js one?

[bitinn]

@BridgeAR I would say both, but I haven't been using the hiredis parser lately. This comment suggests hiredis is affected, but return slightly different results:

#624 (comment)

[BridgeAR]

@bitinn the different behavior is already fixed on master.

As I played around with multi is seems like it's really behaving very inconsistent.

The error is going to land in either the result or the error value depending on what kind of error is thrown.
E.g. if you do not supply enough arguments the parser is going to throw "early" and the reply will be:
[ [Error: Error: EXECABORT Transaction discarded because of previous errors.] ], undefined

But if the command itself is malformed e.g. client.config('abc'); it'll result in:
null, [ 'OK', [Error: ERR CONFIG subcommand must be one of GET, SET, RESETSTAT, REWRITE], 11, 21 ]

The individual command callback in multi behaves just the same. So the error is misplaces just the same.

I suggest to fix this as follows:

1. We create two new error classes for multi statements. One that collects all occuring errors, no matter how many they are and return them as property of the error. And one for an aborted transaction as that won't return any other errors or replies that have already passed.
2. The error is always places correct in the error value
3. The replies are all replies excluding the errors as array

That way it's possible to check if and what error occurred without the need to go through an array of errors and check each individually.

What do you all think?

[bitinn]

Sounds good to me! I am happy as long as the err argument of exec callback always contains an Error object when the transaction failed. So I don't have to do hacks like this: https://github.com/bitinn/decent/blob/master/lib/decent.js#L114-L128

[BridgeAR]

@bitinn I gave this another thought and decided not to break node style err / reply behavior. So the new behavior will be like this:

1. If an EXECABORT error is thrown (any command could not be queued), the error will be in the error parameter (it includes the concrete error occurring). If there's a callback to the specific call, it'll also return the specific error happening. There won't be an array of errors anymore and it'll be of type error! (The later is new).
2. If there's no callback passed to .exec and a EXECABORT error is thrown it'll be emitted (this is new).
3. If something goes wrong while executing the commands (after all got successfully queued) the error will be in the returned result array. It'll be of type error and if there's a callback to the specific function throwing the error it'll be called with that error (this is new - earlier it would have returned null, undefined instead of error, undefined).
   This is needed, since all other commands executed successfully and were not rolled back!

I hope that'll be a good outcome for you.

[bitinn]

@BridgeAR Thx for your work and yes it's a major improvement over what we had.

[brandonros]

return new Promise(function(resolve, reject) {
  client.multi(commands).exec(function(err, res) {
    if (err) {
      return reject(err);
    }

    resolve(res);
  });
});

I don't think reject gets called.

ReplyError: EXECABORT Transaction discarded because of previous errors.
    at parseError (/redacted/node_modules/redis-parser/lib/parser.js:193:12)
    at parseType (/redacted/node_modules/redis-parser/lib/parser.js:303:14)

is thrown instead. Is there no way around that?