EXEC always fails with EXECABORT and multi-state is cleared#7390
EXEC always fails with EXECABORT and multi-state is cleared#7390antirez merged 1 commit intoredis:unstablefrom oranagra:exec_fails_abort
Conversation
|
Thank you @oranagra, given the complexity of this change, please let me review it with care. Thank you :) |
src/networking.c
Outdated
There was a problem hiding this comment.
Hi @oranagra, addReplySds() take ownership of the SDS allocation. I think it's confusing to add this API that does things differently. In general I believe that the "SDS" API should be only low level, where you give it a piece of protocol, in order to compensate all the cases that the API can't handle, but that's all. The other stuff should be handled differently.
There was a problem hiding this comment.
@antirez i'm still not sure what's the best pick here.
I renamed it to addReplyErrorSafe which takes char* and length.
let me know if you have a better idea.
please have a look at the new commit with regards to all your comments.
thanks.
src/server.c
Outdated
There was a problem hiding this comment.
This functions are non obvious in what they are useful for, yet they lack a top comment.
src/server.c
Outdated
There was a problem hiding this comment.
It should be something like: this command is called by processCommand() every time we can't execute a command that is otherwise ready to be executed. The function will flag the transaction if any, so that EXEC fail, and report the error to the client. Or something alone these lines.
src/multi.c
Outdated
There was a problem hiding this comment.
We don't just use ~cmd_flags? The use of an additional field looks a bit too much to me.
There was a problem hiding this comment.
Nevermind, here you want to be sure that all the commands have a specific flag. Looks a good idea to handle LOADING and such, but the comment is not sufficient IMHO.
|
Hi @oranagra, the PR looks good to me mostly, I suggested just a few changes. Thanks. |
|
Thanks @oranagra, that's what I'm doing: merging into unstable, since Redis 6 is safe so far, I would check how the fix will do in unstable for some time and merge into Redis 6 later. Sounds ok? |
|
@oranagra because of this approach, if you agree, I would ask you if you can squash into a single commit so that we can simpler isolate what to merge in Redis 6 later. |
|
@antirez gladly.. i always prefer squashing changes that never made it to any real branch, so we don't see non-final changes in the git log later. |
In order to support the use of multi-exec in pipeline, it is important that MULTI and EXEC are never rejected and it is easy for the client to know if the connection is still in multi state. It was easy to make sure MULTI and DISCARD never fail (done by previous commits) since these only change the client state and don't do any actual change in the server, but EXEC is a different story. Since in the past, it was possible for clients to handle some EXEC errors and retry the EXEC, we now can't affort to return any error on EXEC other than EXECABORT, which now carries with it the real reason for the abort too. Other fixes in this commit: - Some checks that where performed at the time of queuing need to be re- validated when EXEC runs, for instance if the transaction contains writes commands, it needs to be aborted. there was one check that was already done in execCommand (-READONLY), but other checks where missing: -OOM, -MISCONF, -NOREPLICAS, -MASTERDOWN - When a command is rejected by processCommand it was rejected with addReply, which was not recognized as an error in case the bad command came from the master. this will enable to count or MONITOR these errors in the future. - make it easier for tests to create additional (non deferred) clients. - add tests for the fixes of this commit.
|
Thanks! Merged. |
EXEC always fails with EXECABORT and multi-state is cleared
2 participants
In order to support the use of multi-exec in pipeline, it is important that
MULTI and EXEC are never rejected and it is easy for the client to know if the
connection is still in multi state.
It was easy to make sure MULTI and DISCARD never fail (done by previous
commits) since these only change the client state and don't do any actual
change in the server, but EXEC is a different story.
Since in the past, it was possible for clients to handle some EXEC errors and
retry the EXEC, we now can't affort to return any error on EXEC other than
EXECABORT, which now carries with it the real reason for the abort too.
Other fixes in this commit:
validated when EXEC runs, for instance if the transaction contains writes
commands, it needs to be aborted. there was one check that was already done
in execCommand (-READONLY), but other checks where missing: -OOM, -MISCONF,
-NOREPLICAS, -MASTERDOWN
which was not recognized as an error in case the bad command came from the
master. this will enable to count or MONITOR these errors in the future.