Redis Redis should have an option for aborting transactions with EVALSHA inside

Reading a blog post from @stockholmux

https://redislabs.com/blog/bullet-proofing-lua-scripts-in-redispy/

The problem is ofcourse not related to RedisPy or any particular client, but to the protocol allowing an unsafe operation (since transactions can have side effects when part of them fail).

It got me thinking that Redis needs to add an option to abort transactions if a specified list of scripts SHA1 does not exist when starting the transaction itself (since it allows for EVALSHA inside), or maybe just in general, abort if any EVALSHA was not found, which is simpler (depends on the server internals, if it can know before running each command which hashes are inside or not).

This can allow the client to try and be optimistic about trying to send a EVALSHA inside a transaction, and if it fails (the server should return in EXEC a new type of ERROR, such as NOSCRIPTTRANSABORT, or maybe NOSCRIPT), then the client can re-send the transaction this time with EVAL instead of EVALSHA in a safely manner.

Comment From: yossigo

@tzickel Thanks for this idea. It should be fairly easy to detect that an EVALSHA in a transaction refers to a script that does not exist prior to transaction execution and fail the transaction. Also handling the case of EVAL or SCRIPT LOAD of a new script followed by an EVALSHA might be more difficult.

I think that might be a nice safety add-on, but I have some concerns about it breaking backwards compatibility in all kind of odd ways. @oranagra what is your opinion?

Comment From: oranagra

the fix is trivial (although does require writing some ugly code: two loops on the queued commands, checking which commands these are and running a specific cheek for some commands, reaching out from multi.c into scripting.c). IMO the response in that case must be -EXECABORT with some detailed message about the missing script, not sure we can afford to create a new type of error (-NOSCRIPTTRANSABORT)

The other alternative is to run that check while queuing the commands in which case we'll return a -NOSCRIPT instead of +QUEUED, and then return a EXECABORT Transaction discarded because of previous errors for the EXEC.

if we do either of these, i'm not concerned about backwards compatibility, as far as i can tell the only damage it can do is if someone was relying on the fact the transaction will proceed and he'll get the error response of the EVALSHA. i think the chances for that are slim, i don't see why would anyone write such a code, more likely we are just preventing bugs.

thinking of this it brings two other thoughts: 1. maybe there are other type of checks we want to run on commands before executing the transaction? i can't think of any, but i bet there are some that can be done. 2. why would anyone want to use EVALSHA inside MULTI? i think the main reason to use Lua is if you must do things that are impossible with MULTI (i.e. like SET which depend on a result of a GET before it). but if you already bothered to write a script (which is atomic anyway), why would you run it inside a transaction?

Comment From: tzickel

I have proposed in my issue that this will be an option, that way backwards compatibility can be retained, and if someone wants this, he can run maybe MULTI with an optional flag [SAFE] (which maybe can in the future take other considerations that might come up as @oranagra said).

As for why, I am thinking about this from a library developer, if the spec allows it, someone might be trying to abuse it... (and someone has written a long blog post about it in the link above, so maybe it was encountered in the wild).