This is an automated email from the ASF dual-hosted git repository. hulk pushed a commit to branch 2.5 in repository https://gitbox.apache.org/repos/asf/kvrocks.git
commit f4c85b54676780adc14f616c81f5f49abf8f1906 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())
