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

Comments

Projects
None yet
2 participants
@ProgerXP
Contributor

ProgerXP commented Jun 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@badboy

badboy Jun 19, 2016

Collaborator

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

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@ProgerXP

ProgerXP Jun 19, 2016

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ProgerXP

ProgerXP Jun 19, 2016

Contributor

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());
  }
Contributor

ProgerXP commented Jun 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@badboy

badboy Jun 19, 2016

Collaborator

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.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@ProgerXP

ProgerXP Jun 19, 2016

Contributor

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?

Contributor

ProgerXP commented Jun 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ProgerXP

ProgerXP Sep 21, 2016

Contributor

@badboy any reason why my pull request is untouched?

Contributor

ProgerXP commented Sep 21, 2016

@badboy any reason why my pull request is untouched?

@badboy

This comment has been minimized.

Show comment
Hide comment
@badboy

badboy Sep 21, 2016

Collaborator

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

Collaborator

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 added a commit that referenced this issue Sep 22, 2016

Clarified when WATCH doesn't abort EXEC (#737)
* Clarified when WATCH doesn't abort EXEC

 #734 WATCH accounts for modifications by its own connection

* Update transactions.md

@badboy badboy closed this Jul 10, 2017

jeremywadsack added a commit to keylimetoolbox/suo that referenced this issue Aug 30, 2018

Remove #initial_set which is causing a transaction failure and double…
… attempt with delay on lock acquisition #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](antirez/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

Fix `#initial_set` which is causing a double attempt and delay on loc…
…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](antirez/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