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

Order of received commands #91

Closed
lpinca opened this issue Jul 6, 2015 · 6 comments
Closed

Order of received commands #91

lpinca opened this issue Jul 6, 2015 · 6 comments

Comments

@lpinca
Copy link
Contributor

lpinca commented Jul 6, 2015

When using multi or pipeline the order of the commands received by the server is not the expected one:

'use strict';

var Redis = require('ioredis')
  , redis = new Redis();

redis.multi().del('foo').del('bar').exec();
redis.get('foo');
127.0.0.1:6379> MONITOR
OK
1436173662.820473 [0 127.0.0.1:50646] "info"
1436173662.827039 [0 127.0.0.1:50646] "get" "foo"
1436173662.827608 [0 127.0.0.1:50646] "multi"
1436173662.827623 [0 127.0.0.1:50646] "del" "foo"
1436173662.827629 [0 127.0.0.1:50646] "del" "bar"
1436173662.827633 [0 127.0.0.1:50646] "exec"

With node_redis the same code executes the commands in the expected order:

127.0.0.1:6379> MONITOR
OK
1436173811.377020 [0 127.0.0.1:50651] "info"
1436173811.381943 [0 127.0.0.1:50651] "MULTI"
1436173811.382434 [0 127.0.0.1:50651] "del" "foo"
1436173811.382443 [0 127.0.0.1:50651] "del" "bar"
1436173811.382448 [0 127.0.0.1:50651] "EXEC"
1436173811.382520 [0 127.0.0.1:50651] "get" "foo"

Is this wanted?

@luin
Copy link
Collaborator

luin commented Jul 6, 2015

Typically if you want to make sure the execution order of commands, you should either using pipelining or invoking another command after the previous command returning. For instance:

redis.pipeline().multi().del('foo').del('bar').exec().get('foo').exec();

Or

redis.multi().del('foo').del('bar').exec(function () {
  redis.get('foo');
});

This is because although almost all simple commands are sent to the redis server follow the exactly same order they invoked, there are still some exceptions. For example when invoking a custom lua command:

redis.echo();
redis.get('foo');

ioredis first sends evalsha xxx to the redis server and then send get. However if redis returns a NOSCRIPT error for the evalsha command, ioredis will re-send eval xxx instead. So that outwardly echo is sent after get command.

When it comes to multi/exec, ioredis internally queues commands in the other event loop, so it's sent after the latter commands. As for me, I don't like this behaviour and I'm going to make some changes to queue commands in the same event loop. However although not endearing, the current behaviour isn't wrong.

@lpinca
Copy link
Contributor Author

lpinca commented Jul 6, 2015

I understand, thanks for the explanation.
Unfortunately I can't use a callback or a pipeline because I have something like this:

emitter.on('foo', function () {
  redis.multi().del('bar').del('baz').exec();
});

emitter.on('foo', function () {
  redis.get.('bar', function (err, res) {
    if (res) console.log('(屮ಠ益ಠ)屮 Y U STILL HERE');
  });
});

and the two listerners can't be merged because the second one is attached later. A workaround for this particular case is to call redis.get in the next tick, but I'm a bit worried about possible race conditions in a general use case.

@luin
Copy link
Collaborator

luin commented Jul 6, 2015

I'm going to try to change this behaviour at this weekend to make multi works like other simple commands.

BTW just some random workarounds:

emitter.on('foo', function () {
  redis.multi().del('bar').del('baz').exec();
});

emitter.on('foo', function () {
  setImmediate(function () {
    redis.get.('bar', function (err, res) {
      if (res) console.log('(屮ಠ益ಠ)屮 Y U STILL HERE');
    });
  });
});
emitter.on('foo', function () {
  redis.multi().del('bar').del('baz').exec(function () {
    emitter.emit('after foo');
  });
});

emitter.on('after foo', function () {
  redis.get.('bar', function (err, res) {
    if (res) console.log('(屮ಠ益ಠ)屮 Y U STILL HERE');
  });
});
emitter.on('foo', function () {
  redis.multi({ pipeline: false });
  redis.del('bar');
  redis.del('baz');
  redis.exec();
});

emitter.on('foo', function () {
  redis.get.('bar', function (err, res) {
    if (res) console.log('(屮ಠ益ಠ)屮 Y U STILL HERE');
  });
});

@lpinca
Copy link
Contributor Author

lpinca commented Jul 6, 2015

The last example with { pipeline: false }, works well for my use case, thanks.
Just a minor thing: it would be nice if the callback for exec was optional, right now an error is thrown if one is not provided.

'use strict';

var Redis = require('ioredis')
  , redis = new Redis();

redis.multi({ pipeline: false });
redis.del('foo');
redis.del('bar');
redis.exec();
macbook:order luigi$ node index.js 
/Users/luigi/Desktop/order/node_modules/ioredis/node_modules/bluebird/js/main/async.js:43
        fn = function () { throw arg; };
                                 ^
TypeError: undefined is not a function
    at wrapper (/Users/luigi/Desktop/order/node_modules/ioredis/lib/transaction.js:58:7)
    at tryCatcher (/Users/luigi/Desktop/order/node_modules/ioredis/node_modules/bluebird/js/main/util.js:24:31)
    at Promise.successAdapter (/Users/luigi/Desktop/order/node_modules/ioredis/node_modules/bluebird/js/main/nodeify.js:22:30)
    at Promise._settlePromiseAt (/Users/luigi/Desktop/order/node_modules/ioredis/node_modules/bluebird/js/main/promise.js:563:21)
    at Promise._settlePromises (/Users/luigi/Desktop/order/node_modules/ioredis/node_modules/bluebird/js/main/promise.js:681:14)
    at Async._drainQueue (/Users/luigi/Desktop/order/node_modules/ioredis/node_modules/bluebird/js/main/async.js:123:16)
    at Async._drainQueues (/Users/luigi/Desktop/order/node_modules/ioredis/node_modules/bluebird/js/main/async.js:133:10)
    at Immediate.Async.drainQueues [as _onImmediate] (/Users/luigi/Desktop/order/node_modules/ioredis/node_modules/bluebird/js/main/async.js:15:14)
    at processImmediate [as _immediateCallback] (timers.js:367:17)

@luin
Copy link
Collaborator

luin commented Jul 6, 2015

Fixed in 1.5.11. 😄

@luin luin closed this as completed in 8eee7af Jul 7, 2015
@lpinca
Copy link
Contributor Author

lpinca commented Jul 7, 2015

👍

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

No branches or pull requests

2 participants