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

WATCH accounts for modifications by its own connection #734

Closed
ProgerXP opened this issue Jun 19, 2016 · 7 comments
Closed

WATCH accounts for modifications by its own connection #734

ProgerXP opened this issue Jun 19, 2016 · 7 comments

Comments

@ProgerXP
Copy link
Contributor

Documentation about Redis transactions (http://redis.io/topics/transactions) says this:

Optimistic locking using check-and-set

WATCHed keys are monitored in order to detect changes against them. If at least one watched key is modified before the EXEC command, the whole transaction aborts [...]

The above suggests that it doesn't matter who changes the watched key(s) - the same client (that has watched them) or another client.

But the docs have also this fragment:

WATCH explained

[...] we are asking Redis to perform the transaction only if no other client modified any of the WATCHed keys [...]

This explicitly says that WATCH will ignore changes by the same client but doesn't say who is that other client.

I have tried this (single session - single "client"?):

127.0.0.1:6379> watch o
OK
127.0.0.1:6379> set o 1
OK
127.0.0.1:6379> multi
OK
127.0.0.1:6379> get o
QUEUED
127.0.0.1:6379> exec
(nil)

is this a bug in WATCH or the documentation is misleading? Should we remove this "only if no other client" reference from the docs?

@badboy
Copy link
Contributor

badboy commented Jun 19, 2016

At this point I'd say misleading documentation, as the behaviour probably was always that every key modification counts.
cc @antirez

@ProgerXP
Copy link
Contributor Author

ProgerXP commented Jun 19, 2016

After some research I see why the docs mention "other clients":

  • If the same client does watch-set-multi-exec then it fails (as in my first post)
  • If the same client does watch-multi-set-exec - it succeeds
  • If client A does watch, client B does set, client A does multi-exec then it fails
  • If client A does watch, client B does multi-set-exec, client A does multi-exec - it also fails (compare with case 2)

In other words, the docs should say that EXEC will fail in all cases when the key is modified unless it was done by the same client inside the watched multi/exec block.

This might be expected behaviour but why changes before MULTI are treated like a change by another client? I'd say they should be ignored likewise.

@ProgerXP
Copy link
Contributor Author

To summarize my previous post, I'm testing php-redis extension (https://github.com/phpredis/phpredis):

  function testWatchChangeBySelfBeforeMulti() {
    $redis = new Redis;
    $redis->connect('127.0.0.1');
    $redis->delete('o');
    $redis->watch('o');
    $redis->set('o', 1);
    $redis->multi();
    $redis->get('o');
    // This fails.
    $this->assertTrue(is_array( $redis->exec() ));
  }

  function testWatchChangeBySelfAfterMulti() {
    $redis = new Redis;
    $redis->connect('127.0.0.1');
    $redis->delete('o');
    $redis->watch('o');
    $redis->multi();
    $redis->set('o', 1);
    $redis->get('o');
    $this->assertTrue(is_array( $redis->exec() ));
  }

  function testWatchChangeByOtherBeforeMulti() {
    $redis = new Redis;
    $redis->connect('127.0.0.1');

    $redis2 = new Redis;
    $redis2->connect('127.0.0.1');

    $redis->delete('o');
    $redis->watch('o');
    $redis2->set('o', 1);
    $redis->multi();
    $redis->get('o');
    $this->assertFalse($redis->exec());
  }

  function testWatchChangeByOtherAfterMulti() {
    $redis = new Redis;
    $redis->connect('127.0.0.1');

    $redis2 = new Redis;
    $redis2->connect('127.0.0.1');

    $redis->delete('o');
    $redis->watch('o');
    $redis->multi();
    $redis2->set('o', 1);
    $redis->get('o');
    $this->assertFalse($redis->exec());
  }

@badboy
Copy link
Contributor

badboy commented Jun 19, 2016

MULTI/EXEC introduce a transaction. Commands are only executed on EXEC.
Everything before MULTI is executed right away.
WATCH is explicitely used to mark keys used in a transaction. If you edit the key outside of the transaction it is clearly modified, no matter who modified it.
That's why I think the current behaviour is just fine and the documentation can be a bit more clear on this.

@ProgerXP
Copy link
Contributor Author

If you edit the key outside of the transaction it is clearly modified, no matter who modified it.

Then just need to correct the docs. Send you a pull request?

@ProgerXP
Copy link
Contributor Author

@badboy any reason why my pull request is untouched?

@badboy
Copy link
Contributor

badboy commented Sep 21, 2016

Puh, for some reason I failed to review it. I'm sorry for that. Will do it later today.

badboy pushed a commit that referenced this issue Sep 22, 2016
* Clarified when WATCH doesn't abort EXEC

 #734 WATCH accounts for modifications by its own connection

* Update transactions.md
@badboy badboy closed this as completed Jul 10, 2017
jeremywadsack added a commit to keylimetoolbox/suo that referenced this issue Aug 30, 2018
… attempt with delay on lock acquisition nickelser#4

In Redis, calling SET within a WATCH/UNWATCH block but no inside a
MULTI/EXEC block will [cause the EXEC to fail the
transaction](redis/redis-doc#734).
This, along with the call to next after #initial_set in #acquire_lock
is causing the initial acquisition to take two passes and thus
wait for up to :acquisition_delay before getting the lock.

Memcached looks like it will have the same problem as the call to
SET without the CAS during #initial_set is going to cause the SET
with CAS to fail (return EXISTS).
jeremywadsack added a commit to keylimetoolbox/suo that referenced this issue Aug 30, 2018
…k acquisition

The call to `#initial_set` in `#retry` and `#acquire_lock` is followed by `next` which leads to a second pass through the `#retry_with_timeout` loop and a sleep call for up to `:acquisition_delay`. This delay isn't necessary if the value can be set without a race condition.

Removing the `next` call causes the client to continue to retry because the transaction has been changed outside the transaction boundary:

In Redis, calling `SET` within a `WATCH`/`UNWATCH` block but not inside a `MULTI`/`EXEC` block will [cause the EXEC to fail the transaction](redis/redis-doc#734), so the first `#set` call fails and it requires a second pass. To resolve this I changed `#initial_set` to call `#set` within a `MULTI` block so that it would be inside the transaction.

In Memcache the call to `SET` without the `CAS` during `#initial_set` is going to cause the `SET` with `CAS` to fail (return `EXISTS`), and resulting in a second pass. To resolve this I changed `#initial_set` to use `SET` with `CAS` and return the CAS value to be used in the subsequent `#set` call that stores the lock token.
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