From b76016a948ecaf475ddda7958776df9bd5cf9621 Mon Sep 17 00:00:00 2001 From: yoav-steinberg Date: Tue, 8 Feb 2022 11:44:40 +0200 Subject: [PATCH] Consistent erros returned from EVAL scripts (#10218) This PR handles inconsistencies in errors returned from lua scripts. Details of the problem can be found in #10165. ### Changes - Remove double stack trace. It's enough that a stack trace is automatically added by the engine's error handler see https://github.com/redis/redis/blob/d0bc4fff18afdf9e5421cc88e23ffbb876ecaec3/src/function_lua.c#L472-L485 and https://github.com/redis/redis/blob/d0bc4fff18afdf9e5421cc88e23ffbb876ecaec3/src/eval.c#L243-L255 - Make sure all errors a preceded with an error code. Passing a simple string to `luaPushError()` will prepend it with a generic `ERR` error code. - Make sure lua error table doesn't include a RESP `-` error status. Lua stores redis error's as a lua table with a single `err` field and a string. When the string is translated back to RESP we add a `-` to it. See https://github.com/redis/redis/blob/d0bc4fff18afdf9e5421cc88e23ffbb876ecaec3/src/script_lua.c#L510-L517 So there's no need to store it in the lua table. ### Before & After ```diff --- +++ @@ -1,14 +1,14 @@ 1: config set maxmemory 1 2: +OK 3: eval "return redis.call('set','x','y')" 0 - 4: -ERR Error running script (call to 71e6319f97b0fe8bdfa1c5df3ce4489946dda479): @user_script:1: @user_script: 1: -OOM command not allowed when used memory > 'maxmemory'. + 4: -ERR Error running script (call to 71e6319f97b0fe8bdfa1c5df3ce4489946dda479): @user_script:1: OOM command not allowed when used memory > 'maxmemory'. 5: eval "return redis.pcall('set','x','y')" 0 - 6: -@user_script: 1: -OOM command not allowed when used memory > 'maxmemory'. + 6: -OOM command not allowed when used memory > 'maxmemory'. 7: eval "return redis.call('select',99)" 0 8: -ERR Error running script (call to 4ad5abfc50bbccb484223905f9a16f09cd043ba8): @user_script:1: ERR DB index is out of range 9: eval "return redis.pcall('select',99)" 0 10: -ERR DB index is out of range 11: eval_ro "return redis.call('set','x','y')" 0 -12: -ERR Error running script (call to 71e6319f97b0fe8bdfa1c5df3ce4489946dda479): @user_script:1: @user_script: 1: Write commands are not allowed from read-only scripts. +12: -ERR Error running script (call to 71e6319f97b0fe8bdfa1c5df3ce4489946dda479): @user_script:1: ERR Write commands are not allowed from read-only scripts. 13: eval_ro "return redis.pcall('set','x','y')" 0 -14: -@user_script: 1: Write commands are not allowed from read-only scripts. +14: -ERR Write commands are not allowed from read-only scripts. ``` --- src/script_lua.c | 26 +++++++++------ tests/unit/scripting.tcl | 72 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 10 deletions(-) diff --git a/src/script_lua.c b/src/script_lua.c index 2873159d4..82591d3fc 100644 --- a/src/script_lua.c +++ b/src/script_lua.c @@ -432,7 +432,7 @@ static void redisProtocolToLuaType_Double(void *ctx, double d, const char *proto * table is never a valid reply by proper commands, since the returned * tables are otherwise always indexed by integers, never by strings. */ void luaPushError(lua_State *lua, char *error) { - lua_Debug dbg; + sds msg; /* If debugging is active and in step mode, log errors resulting from * Redis commands. */ @@ -443,15 +443,21 @@ void luaPushError(lua_State *lua, char *error) { lua_newtable(lua); lua_pushstring(lua,"err"); - /* Attempt to figure out where this function was called, if possible */ - if(lua_getstack(lua, 1, &dbg) && lua_getinfo(lua, "nSl", &dbg)) { - sds msg = sdscatprintf(sdsempty(), "%s: %d: %s", - dbg.source, dbg.currentline, error); - lua_pushstring(lua, msg); - sdsfree(msg); - } else { - lua_pushstring(lua, error); - } + /* There are two possible formats for the received `error` string: + * 1) "-CODE msg": in this case we remove the leading '-' since we don't store it as part of the lua error format. + * 2) "msg": in this case we prepend a generic 'ERR' code since all error statuses need some error code. + * We support format (1) so this function can reuse the error messages used in other places in redis. + * We support format (2) so it'll be easy to pass descriptive errors to this function without worrying about format. + */ + if (error[0] == '-') + msg = sdsnew(error+1); + else + msg = sdscatprintf(sdsempty(), "ERR %s", error); + /* Trim newline at end of string. If we reuse the ready-made Redis error objects (case 1 above) then we might + * have a newline that needs to be trimmed. In any case the lua Redis error table shouldn't end with a newline. */ + msg = sdstrim(msg, "\r\n"); + lua_pushstring(lua, msg); + sdsfree(msg); lua_settable(lua,-3); } diff --git a/tests/unit/scripting.tcl b/tests/unit/scripting.tcl index 93db6d071..cfbe60faf 100644 --- a/tests/unit/scripting.tcl +++ b/tests/unit/scripting.tcl @@ -1400,3 +1400,75 @@ start_server {tags {"scripting"}} { set _ {} } {} {external:skip} } + +# Additional eval only tests +start_server {tags {"scripting"}} { + test "Consistent eval error reporting" { + r config set maxmemory 1 + # Script aborted due to Redis state (OOM) should report script execution error with detailed internal error + assert_error {ERR Error running script (call to *): @user_script:*: OOM command not allowed when used memory > 'maxmemory'.} { + r eval {return redis.call('set','x','y')} 1 x + } + # redis.pcall() failure due to Redis state (OOM) returns lua error table with Redis error message without '-' prefix + assert_equal [ + r eval { + local t = redis.pcall('set','x','y') + if t['err'] == "OOM command not allowed when used memory > 'maxmemory'." then + return 1 + else + return 0 + end + } 1 x + ] 1 + # Returning an error object from lua is handled as a valid RESP error result. + assert_error {OOM command not allowed when used memory > 'maxmemory'.} { + r eval { return redis.pcall('set','x','y') } 1 x + } + r config set maxmemory 0 + # Script aborted due to error result of Redis command + assert_error {ERR Error running script (call to *): @user_script:*: ERR DB index is out of range} { + r eval {return redis.call('select',99)} 0 + } + # redis.pcall() failure due to error in Redis command returns lua error table with redis error message without '-' prefix + assert_equal [ + r eval { + local t = redis.pcall('select',99) + if t['err'] == "ERR DB index is out of range" then + return 1 + else + return 0 + end + } 0 + ] 1 + # Script aborted due to scripting specific error state (write cmd with eval_ro) should report script execution error with detailed internal error + assert_error {ERR Error running script (call to *): @user_script:*: ERR Write commands are not allowed from read-only scripts.} { + r eval_ro {return redis.call('set','x','y')} 1 x + } + # redis.pcall() failure due to scripting specific error state (write cmd with eval_ro) returns lua error table with Redis error message without '-' prefix + assert_equal [ + r eval_ro { + local t = redis.pcall('set','x','y') + if t['err'] == "ERR Write commands are not allowed from read-only scripts." then + return 1 + else + return 0 + end + } 1 x + ] 1 + } {} {cluster:skip} + + test "LUA redis.error_reply API" { + assert_error {MY_ERR_CODE custom msg} { + r eval {return redis.error_reply("MY_ERR_CODE custom msg")} 0 + } + } + + test "LUA redis.status_reply API" { + r readraw 1 + assert_equal [ + r eval {return redis.status_reply("MY_OK_CODE custom msg")} 0 + ] {+MY_OK_CODE custom msg} + r readraw 0 + } +} +