WATCH during MULTI shouldn't fail transaction

[jabolina]

Bug Report

Redis returns an error if a WATCH command is submitted inside a MULTI. However, this command is silently discarded. The WATCH command does not count in the final result list during the EXEC phase. For example:

127.0.0.1:6379> multi
OK
127.0.0.1:6379(TX)> set foo fooer
QUEUED
127.0.0.1:6379(TX)> watch some-key
(error) ERR WATCH inside MULTI is not allowed
127.0.0.1:6379(TX)> set bar baer
QUEUED
127.0.0.1:6379(TX)> exec
1) OK
2) OK

Observe that the WATCH command is not included in the result list.

Current Behavior

Lettuce includes the failed WATCH command in the output list. Additionally, since the command is dropped, the response list has fewer responses than the command list in MultiOutput and some commands might never be completed.

Input Code

For example, adding the following test to the TransactionCommandIntegrationTests class:

Input Code

    @Test
    void watchWithinMulti() {
        redis.multi();
        redis.set("one-a", "a");
        redis.set("one-b", "b");
        redis.watch("something");
        redis.set("one-c", "c");

        TransactionResult result = redis.exec();
        for (Object o : result) {
            System.out.println(o);
        }

        assertThat(result)
                .hasSize(3)
                .allMatch("OK"::equals);
    }

Produces:

OK
OK
io.lettuce.core.RedisCommandExecutionException: ERR WATCH inside MULTI is not allowed

java.lang.AssertionError: 
Expecting all elements of:
  DefaultTransactionResult [wasRolledBack=false, responses=3]
to match given predicate but this element did not:
  io.lettuce.core.RedisCommandExecutionException: ERR WATCH inside MULTI is not allowed
	at io.lettuce.core.internal.ExceptionFactory.createExecutionException(ExceptionFactory.java:152)
	at io.lettuce.core.internal.ExceptionFactory.createExecutionException(ExceptionFactory.java:120)
	at io.lettuce.core.output.MultiOutput.complete(MultiOutput.java:154)
	...(25 remaining lines not displayed - this can be changed with Assertions.setMaxStackTraceElementsDisplayed)

	at io.lettuce.core.commands.TransactionCommandIntegrationTests.watchWithinMulti(TransactionCommandIntegrationTests.java:135)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

Expected behavior/code

The test should complete successfully with all 3 set operations returning an OK response.

Environment

• Lettuce version(s): main branch

Additional context

I think WATCH is the only command that the MULTI will drop. The solution will require something specific to WATCH.

[tishun]

I think WATCH is the only command that the MULTI will drop. The solution will require something specific to WATCH.

Not only, unfortunately. MULTI inside MULTI is not allowed too (and does not fail the transaction) :

127.0.0.1:6484> MULTI
OK
127.0.0.1:6484(TX)> SET a b
QUEUED
127.0.0.1:6484(TX)> MULTI
(error) ERR MULTI calls can not be nested
127.0.0.1:6484(TX)> SET b b
QUEUED
127.0.0.1:6484(TX)> EXEC
1) OK
2) OK

[tishun]

The test should complete successfully with all 3 set operations returning an OK response.

I agree that the last result should not be omitted from the TransactionResult, but I disagree that the error should be silently ignored. After all the transaction was supposed to contain 4 commands and, respectively, I would assume it would have 4 results (for each of the commands). Otherwise it might not be immediately visible to the user where the result from the WATCH command is, or why it failed for that matter.

[tishun]

This issue proved to be much more convoluted than expected.

My final proposal (in #3027) is to make WATCH behave as MULTI does currently. Nesting a MULTI inside a MULTI raises an exception, but the user is free to catch it and it would not fail the transaction. This is - more or less - similar to what the redis-cli would do.

One could argue that we should not raise an exception, but I would have to disagree for two main reasons.
Firstly - unless they inspect the result - users might not know they are doing something wrong and would expect for the watch to work. In second place the output of individual commands is not inspected (in all other cases) until the EXEC is called.

On a broader note I think this is a deficiency of the server - the current way it works it behaves in three different ways:

• fails the transaction (if for ex. a malformed command is provided inside a transaction)
• does not fail the transaction, but includes errors in the output after EXEC (in case the command returns an error response, e.g. CLUSTER SLOTS called in a non-clustered environment)
• does not fail the transaction, but does not include the error in the output after EXEC (nested MULTI and WATCH).

[jabolina]

Thanks for the work, @tishun!

[tishun]

Thanks for the work, @tishun!

Let me know if we can improve the solution in any way