This is an automated email from the ASF dual-hosted git repository.
hulk pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new 2c3aed36 Fix EXEC / DISCARD without MULTI wrongly reset watch (#1562)
2c3aed36 is described below
commit 2c3aed365a89794fc69e05c68d61011eea908206
Author: Binbin <[email protected]>
AuthorDate: Tue Jul 11 19:43:52 2023 +0800
Fix EXEC / DISCARD without MULTI wrongly reset watch (#1562)
The old code called ResetWatchedKeys before the error, and then
ResetWatchedKeys reset the watched_keys_modified flag which causes
the following inconsistencies (with Redis).
kvrocks output:
```
127.0.0.1:6666> watch a
OK
127.0.0.1:6666> watch b ->>> other client touch a
OK
127.0.0.1:6666> exec
(error) ERR EXEC without MULTI
127.0.0.1:6666> multi
OK
127.0.0.1:6666(TX)> ping
QUEUED
127.0.0.1:6666(TX)> exec
1) PONG
```
redis output:
```
127.0.0.1:6379> watch a
OK
127.0.0.1:6379> watch b ->>> other client touch a
OK
127.0.0.1:6379> exec
(error) ERR EXEC without MULTI
127.0.0.1:6379> multi
OK
127.0.0.1:6379(TX)> ping
QUEUED
127.0.0.1:6379(TX)> exec
(nil)
````
Introduced in #1279
---
src/commands/cmd_txn.cc | 7 +++----
tests/gocase/unit/multi/multi_test.go | 32 ++++++++++++++++++++++++++++++++
2 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/src/commands/cmd_txn.cc b/src/commands/cmd_txn.cc
index 19eb7005..8dbaa24d 100644
--- a/src/commands/cmd_txn.cc
+++ b/src/commands/cmd_txn.cc
@@ -45,14 +45,14 @@ class CommandMulti : public Commander {
class CommandDiscard : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- auto reset_watch = MakeScopeExit([svr, conn] {
svr->ResetWatchedKeys(conn); });
-
if (!conn->IsFlagEnabled(Connection::kMultiExec)) {
*output = redis::Error("ERR DISCARD without MULTI");
return Status::OK();
}
+ auto reset_watch = MakeScopeExit([svr, conn] {
svr->ResetWatchedKeys(conn); });
conn->ResetMultiExec();
+
*output = redis::SimpleString("OK");
return Status::OK();
@@ -62,13 +62,12 @@ class CommandDiscard : public Commander {
class CommandExec : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- auto reset_watch = MakeScopeExit([svr, conn] {
svr->ResetWatchedKeys(conn); });
-
if (!conn->IsFlagEnabled(Connection::kMultiExec)) {
*output = redis::Error("ERR EXEC without MULTI");
return Status::OK();
}
+ auto reset_watch = MakeScopeExit([svr, conn] {
svr->ResetWatchedKeys(conn); });
auto reset_multiexec = MakeScopeExit([conn] { conn->ResetMultiExec(); });
if (conn->IsMultiError()) {
diff --git a/tests/gocase/unit/multi/multi_test.go
b/tests/gocase/unit/multi/multi_test.go
index 011b922d..b2de3780 100644
--- a/tests/gocase/unit/multi/multi_test.go
+++ b/tests/gocase/unit/multi/multi_test.go
@@ -177,6 +177,22 @@ func TestMulti(t *testing.T) {
require.NoError(t, rdb.Do(ctx, "EXEC").Err())
})
+ t.Run("EXEC without MULTI is not allowed", func(t *testing.T) {
+ require.EqualError(t, rdb.Do(ctx, "EXEC").Err(), "ERR EXEC
without MULTI")
+ })
+
+ t.Run("EXEC without MULTI should not reset watch", func(t *testing.T) {
+ rdb2 := srv.NewClient()
+ defer func() { require.NoError(t, rdb2.Close()) }()
+
+ require.NoError(t, rdb.Do(ctx, "WATCH", "x").Err())
+ require.NoError(t, rdb2.Do(ctx, "SET", "x", "xxx").Err())
+ require.EqualError(t, rdb.Do(ctx, "EXEC").Err(), "ERR EXEC
without MULTI")
+ require.NoError(t, rdb.Do(ctx, "MULTI").Err())
+ require.NoError(t, rdb.Do(ctx, "PING").Err())
+ require.Equal(t, rdb.Do(ctx, "EXEC").Val(), nil)
+ })
+
t.Run("EXEC works on WATCHed key not modified", func(t *testing.T) {
require.NoError(t, rdb.Do(ctx, "WATCH", "x", "y", "z").Err())
require.NoError(t, rdb.Do(ctx, "WATCH", "k").Err())
@@ -268,6 +284,22 @@ func TestMulti(t *testing.T) {
require.Equal(t, rdb.Do(ctx, "EXEC").Val(), nil)
})
+ t.Run("DISCARD without MULTI is not allowed", func(t *testing.T) {
+ require.EqualError(t, rdb.Do(ctx, "DISCARD").Err(), "ERR
DISCARD without MULTI")
+ })
+
+ t.Run("DISCARD without MULTI should not reset watch", func(t
*testing.T) {
+ rdb2 := srv.NewClient()
+ defer func() { require.NoError(t, rdb2.Close()) }()
+
+ require.NoError(t, rdb.Do(ctx, "WATCH", "x").Err())
+ require.NoError(t, rdb2.Do(ctx, "SET", "x", "xxx").Err())
+ require.EqualError(t, rdb.Do(ctx, "DISCARD").Err(), "ERR
DISCARD without MULTI")
+ require.NoError(t, rdb.Do(ctx, "MULTI").Err())
+ require.NoError(t, rdb.Do(ctx, "PING").Err())
+ require.Equal(t, rdb.Do(ctx, "EXEC").Val(), nil)
+ })
+
t.Run("DISCARD should clear the WATCH dirty flag on the client", func(t
*testing.T) {
require.NoError(t, rdb.Set(ctx, "x", 30, 0).Err())
require.NoError(t, rdb.Do(ctx, "WATCH", "x").Err())
