Making two operations atomic when the second depends on the result of the first.

[ericrini]

Can someone clarify for me if this would execute atomically, such that if the command was executed simultaneously in two places, there would not be a race to write back the new value.

Here is an example of the code, note that the write value is dependent on the read value.

client.multi([
    ["hget", {orgId}, {topicId}]
]).exec(function (err, sessions) {
    sessions = JSON.parse(sessions);
    if (sessions === undefined) {
        sessions = [sessionId];
    }
    else {
        sessions.push(sessionId)
    }
    client.hset({orgId}, {topicId}, JSON.stringify(sessions));
});

[brycebaril]

Hi @ericrini -- unfortunately this is not how multi works. This would not be atomic between the two operations.

I suggest reading up on Redis transactions here: http://redis.io/topics/transactions

[mranney]

Check out the "Optimistic locking using check-and-set" section here:

http://redis.io/topics/transactions

You probably need the WATCH command to get the functionality that you want. Note that this command applies to the connection, so if you have multiple "users" inside of the same process, they could step on each other until the transaction finishes.

[mranney]

Looks like we need some transaction support on the github commenting system.

[brycebaril]

Awesome timing! 😃

[ericrini]

So a complete solution would look like this... retrying 3 times and making sure each client is isolated to its own connection within the same process.

var atomicReadWriteOperationRecursive = function (hashKey, valueKey, count) {
    var client = redis.createClient();
    client.watch(hashKey, valueKey);
    client.hget(hashKey, valueKey, function (err, response) {
            client.multi()
                .hset(orgId, topicId, newValue);
                .exec(function (err) {
                    client.close();
                    if (err && count < 3) {
                        atomicReadWriteOperationRecursive(hashKey, valueKey, count++);  
                    }
                });
        });
}

Sorry I realize I edited that like 5 times after you replied.

[brycebaril]

Not quite -- the WATCH must happen outside of a MULTI transaction to work.

client.watch(orgId)
client.hget(orgId, topicId, function (err, response) {
  // logic
  client.multi()
    .hset(orgId, topicId, newValue)
    .exec(function (err, reply) {
      // handle error, reply
    })
})

That said, I'd suggest trying it out on the command line with redis-cli first to make sure you understand what is going on here, and then definitely test your code well to make sure you handle the errors and other issues.

Using WATCH is relatively advanced and carries some other implications that aren't obvious with this library. Due to the fact that node is non-blocking the client can interfere with itself between the WATCH and MULTI command, e.g. you try to start a WATCH on a new key... It may be advisable to create a new client on actions that require a WATCH.

That's one thing we'd like to address in future versions of this library, we'll likely want a sort of connection pool for WATCH/MULTI check-and-set transactions. TBD.

Another option if your logic is self contained and simple enough is to use LUA scripting which is entirely atomic per LUA command.

[bberry6]

So I have been toying with the WATCH functionality and have found that it isn't obvious when the transaction aborted (due to the watched key being modified) since the error parameter in the callback function returns "null" whether the transaction aborted or not...

Here's a little node script...

// foo.js
(function(){
   'use strict';

   var Bluebird = require('bluebird');
   var redis = require('redis');
   var client = redis.createClient();
   client.on("error", function(err){
      console.error("RedisService::createClient => Error in client creation: ", err);
   });

   client.watch('foo');
   wait()
   .then(doTransaction)
   .then(function(){
      process.exit();
   });

   function doTransaction(){
      return new Bluebird(function(resolve, reject){
         client.multi()
         .hset('foo', 'bar', 10)
         .hgetall('foo')
         .exec(function(err, results){
            console.log('err: ',err);
            console.log('results: ', results);
            client.quit();
            resolve();
         });
      });
   }
   function wait(){
      return new Bluebird(function(resolve, reject){
         setTimeout(function(){
            console.log('timeout complete');
            resolve();
         },3000);
      });
   }

})();

For testing, I begin by watching foo , then I wait for 3 seconds (explained below). Then I run the multi() and print the error and results parameters, quit the client, and then kill the process.

Here's the output:

bberry:~$ node foo.js
timeout complete
err:  null
results:  [ 0, { bar: '10' } ]

Looks great!
Now I try the test again, except this time during the 3 second period, I am going to use redis-cli to run hset foo bar 5.

Here's the output:

bberry:~$ node foo.js
timeout complete
err:  null
results:  null

The question is... in the exec callback function, how should we check if the transaction was aborted and try again? Should we check if the results array returns null? It seems like the err is used for redis syntax and other errors instead of the aborting of transactions.

[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.

[BridgeAR]

@bberry6 is this still reproducable on >= v.2.0?

[bberry6]

@BridgeAR yep, I just repeated the steps that I described in my previous post and receive the same output. I'm on redis@2.2.3

brett@Bretts-MacBook-Pro:footest$ npm install redis
redis@2.2.3 node_modules/redis
└── double-ended-queue@2.1.0-0
brett@Bretts-MacBook-Pro:footest$ npm install bluebird
bluebird@2.10.2 node_modules/bluebird
brett@Bretts-MacBook-Pro:footest$ node foo.js
timeout complete
err:  null
results:  [ 1, { bar: '10' } ]
brett@Bretts-MacBook-Pro:footest$ node foo.js
timeout complete
err:  null
results:  null

[BridgeAR]

@bberry6 would you be so kind and also run this with the hiredis parser?

[BridgeAR]

@bberry6 So I took the time to read about watch and it is working as expected. The result is null to indicate that the transaction has not been executed at all. I think it's surprising that it's not returned as an error instead, but this is very likely due to signal that everything worked as expected and no "error" occurred and still tell the user that the transaction aborted.

@ericrini you expect an error in the return value to signal the transaction aborted. As I just stated,you have to check if the result is null instead.
Besides that the count is never incremented in your example, as it uses a post-increment instead of a pre-increment and the same number is used in every call and you can't watch fields of a hash but only keys.

I did not test this but this code should work as a atomic read / write operation that adds values to a existing key or creates that key with the provided value:

var client = redis.createClient();

var atomicReadWriteOperationRecursive = function (key, addValue, callback, count) {
    count = count | 0; // Set count with a default value of zero by converting the value to a SMI
    client.watch(key);
    client.get(key, function (err, response) {
        if (err) return callback(err);
        var newValue = reponse ? JSON.parse(response) : [];
        newValue.push(addValue);
        client.multi()
            .set(key, JSON.stringify(newValue));
            .exec(function (err, res) {
                if (err) return callback(err);
                if (res) return callback(null, 'OK');
                if (count < 10) atomicReadWriteOperationRecursive(key, addValue, callback, ++count);
                else callback(new Error('Atomic read write failed'));
            });
    });
}

atomicReadWriteOperationRecursive(orgId + ':' + topicId, sessionId, function (err, res) {
    if (err) throw err;
    // handle result of a successfull atomic read write operation
});

[ericrini]

Just a thought for anyone else struggling with similar problems.

I wrote this question a long time ago as a relative Redis novice (as can be seen by the numerous mistakes in the original idea). Ultimately, I solved the original issue by switching to a different Redis data structure that changed the problem in a way that could avoid this entirely. Since then, I've talked to quite a few other devs who had similar experiences where changing the data structure allowed them to greatly simplify the actual problem in a way that avoided transactions or lua scripting. Not saying there's never a reason to do this, but definitely make sure there's no other options on the table. :)

Thanks you everyone for all your great support.