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.

Sign up for GitHub

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

Jump to bottom

WATCH during MULTI shouldn't fail transaction #3009

Open
jabolina opened this issue Oct 10, 2024 · 3 comments · May be fixed by #3027
Open

WATCH during MULTI shouldn't fail transaction #3009

jabolina opened this issue Oct 10, 2024 · 3 comments · May be fixed by #3027
Labels
type: bug A general bug

Comments

@jabolina
Copy link
Contributor

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 tishun added for: team-attention An issue we need to discuss as a team to make progress status: waiting-for-triage labels Oct 11, 2024
@tishun
Copy link
Collaborator

tishun commented Oct 11, 2024

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 tishun added the type: bug A general bug label Oct 11, 2024
@tishun
Copy link
Collaborator

tishun commented Oct 11, 2024

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 tishun linked a pull request Oct 24, 2024 that will close this issue
Open
4 tasks
@tishun
Copy link
Collaborator

tishun commented Oct 25, 2024

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).

@tishun tishun removed for: team-attention An issue we need to discuss as a team to make progress status: waiting-for-triage labels Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: bug A general bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WATCH during MULTI shouldn't fail transaction #3009 tishun/lettuce-core
2 participants
@tishun @jabolina