Fix MULTI transactions calling the block twice

[KJTsanaktsidis]

What this PR does

Currently using a transaction with RedisClient::Cluster#multi actually calls the provided block twice: Once in RedisClient::Cluster::Transaction#execute to work out what node to send the transaction to, and then once again in RedisClient#multi to actually execute the thing. Also, the first time the block is called, the keys are not watched yet, so any get calls you make in there are stale.

This PR fixes the issue by building up a buffer of transaction commands to execute when yielding the block passed to #multi, and then yielding that buffer into RedisClient#multi instead of the user-provided block again. That way, the block only gets called once.

This PR also adds in some machinery to make sure that any commands run on the client during a #multi block happen on the primary; this is almost always what you want to be doing inside a transaction. If you watch a key on a primary but then read it from the replica, it's possible for you to read a stale value that was already known to the primary when you called WATCH; you will then build up a transaction from stale data and your EXEC will succeed where you would expect it to fail.

With this PR, it's possible to do e.g. an atomic swap of two variables with something like this:

$redis = RedisClient::Cluster.new
$redis.with(watch: ['{hash}key1', {hash}key2']) do |txn|
    old_value_1 = $redis.call_v(['GET', '{hash}key1'])
    old_value_2 = $redis.call_v(['GET', '{hash}key2'])
    txn.call_v(['SET', '{hash}key1', old_value_2])
    txn.call_v(['SET', '{hash}key2', old_value_1])
end

Note, though, that it's not possible to actually pass watch: keys to multi if you're using redis-cluster-client through the redis-rb and redis-clustering gems. That should be a very simple fix however: redis/redis-rb#1236

What this PR doesn't do

Even with this change, using Redis::Cluster#multi from redis-clustering/redis-rb is pretty wonky. Something like this does work and swap the keys:

$redis = Redis::Cluster.new
$redis.set('{hash}key1', 'value1')
$redis.set('{hash}key2', 'value2')

$redis.watch(['{hash}key1', '{hash}key2']) do
  orig_key1 = $redis.get '{hash}key1'
  orig_key2 = $redis.get '{hash}key2'
  $redis.multi do |txn|
    txn.set '{hash}key1', orig_key2
    txn.set '{hash}key2', orig_key1
  end
end

But, support for unwatching and not committing the transaction is broken. This throws RedisClient::Cluster::AmbiguousNodeError because the router doesn't know what node to send UNWATCH to.

$redis = Redis::Cluster.new
$redis.set('{hash}key1', 'value1')
$redis.set('{hash}key2', 'value2')

$redis.watch(['{hash}key1', '{hash}key2']) do
  orig_key1 = $redis.get '{hash}key1'
  orig_key2 = $redis.get '{hash}key2'
  # whelp, we didn't like what we read from our gets.
  $redis.unwatch
end

This happens because there's nothing storing the idea that a transaction is happening around the entire watch block; the Redis::Cluster::Transaction is only created around a call to #multi. On a similar note, nothing stops you from watching or getting keys from multiple different hash slots in your transaction. For example, this will work, but shouldn't.

$redis = Redis::Cluster.new
$redis.set('{hash1}key1', 'value1')
$redis.set('{hash2}key2', 'value2')
$redis.watch(['{hash1}key1']) do
  $redis.watch '{hash2}key2'
  orig_key1 = $redis.get '{hash1}key1'
  orig_key2 = $redis.get '{hash2}key2'

  # some other process changes the value of key2 after we read it
  Thread.new { Redis::Cluster.new.set('{hash2}key2', 'hahahaha') }.join

  # This will commit, but shouldn't, because we're not _really_ watching key2; it's on another node
  # to the one with the transaction
  $redis.multi do |txn|
    txn.set '{hash}key1', orig_key1 + orig_key2
  end
end

I played around for a couple of days trying to work out the right way to fix this, but didn't really land on anything I like. I'm open to ideas if you can think of any good way to do this though. The closest I came up with was this idea about maintaining some transaction state in RedisCluster::Client, but the thing that makes it tricky is that Redis::Cluster#watch (which is actually Redis::Commands::Transactions#watch) just does call_v([:watch] + keys]) and then send_command([:unwatch]) in a begin/rescue block; it doesn't pass the watch block to RedisClient::Cluster at all to invoke which means it's difficult to maintain our own state reliably in redis-clustering.

Anyway, as I said, if you can think of a way to make this work nicely I'm all ears.

Thanks for your review!

[supercaracal]

Thank you for your pull request. Please give me some time to look into it.

[KJTsanaktsidis]

Thank you @supercaracal - of course, this stuff is incredibly complicated so it's worth going over carefully!

Just as an update - I had another go at fixing the "What this PR doesn't do" problem with using redis-cluster-client through redis-rb's #watch and #multi method above. I made a draft PR which solves both problems with a different approach: #295

The approach in that PR is to add the idea of an "implicit transaction". Essentially, the client starts a RedisClient::Cluster::Transaction instance when a WATCH or MULTI command is issued, and discards that when a EXEC, DISCARD or UNWATCH is issued. The RedisClient::Cluster keeps track of whether or not it has an active transaction; if it does, it routes commands to the transaction instead of to the @router directly.

The RedisClient::Cluster::Transaction object accepts the commands and sends them to a single Redis connection to a single Redis node. Any cross-node access whilst a transaction is active on the connection results in a TransactionConsistencyError.

That PR makes my not-working example above actually work; I wrote a series of tests in redis-rb which will pass with that other PR: redis/redis-rb@master...zendesk:redis-rb:ktsanaktsidis/cluster_txn_tests

The "implicit transaction" approach certainly adds some complexity to the implementation of redis-cluster-client. The interface between redis-rb and redis-client for single-node watch is really oriented around the idea that the connection itself carries the implicit state as to whether it's watching something or not. I think the idea of storing an "implicit transaction" inside the RedisClient::Cluster object is the easiest way to make redis-cluster-client mimic this interface. I grant you that it is a very strange interface though.

A better interface for doing watches with cluster client is something like this, I think:

$redis.watch do |txn|
    txn.watch "{slot}key1"
    x = txn.get("{slot}key1")
    txn.multi do |pipeline|
        pipeline.set "{slot}key1" x + 1
    end
end

That makes the implicit state of the transaction created by watch actually explicit! However, that's not how redis-rb or redis-client work, so obviously this would be a breaking change and a total non-starter.

---

tl;dr: should we consider the approach in #295 instead?

[supercaracal]

I'd say that the redis-cluster-client should be simple. So I'd like to allow only an interface for transactions feature by single-method calling with a block. Also I want to deny an independent calling by a WATCH command with throwing an error. Of course the redis-cluster-client should be aware of the redis-clustering usability, but I'd say that it would be better to prevent to be affected by the design too much.

I think your idea to use #with method is good. The redis-clustering gem aside, how about the following example of the interface?

redis = RedisClient.cluster.new_client

redis.with(key: '{key}1', primary: true) do |r|
  # r is an instance of RedisClient
  r.multi(watch: %w[{key}1 {key}2]) do |tx|
    # tx is implemented by RedisClient
    v1 = r.call('GET', '{key}1')
    v2 = r.call('GET', '{key}2')
    tx.call('SET', '{key}1', v2)
    tx.call('SET', '{key}2', v1)
  end
end

redis.multi(watch: %w[{key}1 {key}2]) do |tx|
  # tx is implemented by RedisClient::Cluster
  tx.call('INCR', '{key}1')
  tx.call('INCR', '{key}2')
end

[KJTsanaktsidis]

I think your idea to use #with method is good.

Actually I realise it was a mistake in my original message. When I wrote $redis.with, I meant $redis.multi. So, the example above (which this PR does make work) should be:

$redis = RedisClient::Cluster.new
$redis.multi(watch: ['{hash}key1', {hash}key2']) do |txn|
    old_value_1 = $redis.call_v(['GET', '{hash}key1'])
    old_value_2 = $redis.call_v(['GET', '{hash}key2'])
    txn.call_v(['SET', '{hash}key1', old_value_2])
    txn.call_v(['SET', '{hash}key2', old_value_1])
end

how about the following example of the interface?

The idea is that calling #with yields a RedisClient which is locked to a particular node of the cluster, right? I think this is actually a really good idea (and way better than my idea).

I guess the implementation would be that RedisClient::Cluster#with yields a proxy object, which delegates all accesses to a particular underlying RedisClient right? The proxy object would be able to verify that all keys accesed through it are for the same slot (we could just send the commands to the Redis server without checking, but the Redis server will only complain about keys if they are on the wrong node; this way we can make sure people don't write transactions that accidently work until they scale out their cluster).

The really good thing about this is that RedisClient defines #with as:

  def with(_options = nil)
    yield self
  end

i.e. it accepts and ignores options; so, you can easily write code which is compatible with both RedisClient and RedisClient::Cluster - just call with(key: ...) unconditionally, which will be a no-op on non-clustreed redis.

Regarding redis-clustering, I think if we have this interface in redis-cluster-client, we can implement a Redis::Cluster#watch and Redis::Cluster#multi method in in redis-clustering which calls RedisClient::Cluster#with as you described in your example. This would allow us to implement at least the block-based form of Redis::Cluster#watch properly.

I'll play around with implementing this + the redis-cluster-client side this week and open another PR so we can compare the approach - does that work for you?

Thanks so much for giving this some deep thought by the way!

[supercaracal]

We are on the same page.

does that work for you?

Yes, it does. Thank you so much!