Skip to content

Script reply errors cleanup and stats #10279

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

Closed
oranagra wants to merge 1 commit into from
+50 −18
Closed

Script reply errors cleanup and stats #10279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/eval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ void scriptingInit(int setup) {
" i = dbg.getinfo(3,'nSl')\n"
" end\n"
" if i then\n"
" return i.source .. ':' .. i.currentline .. ': ' .. err\n"
" return err .. '. ' .. i.source .. ':' .. i.currentline\n"
Copy link
Contributor

@yoav-steinberg yoav-steinberg Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is a bit nicer:

Suggested change
" return err .. '. ' .. i.source .. ':' .. i.currentline\n"
" return err .. ' (' .. i.source .. ':' .. i.currentline .. ')'\n"

" else\n"
" return err\n"
" end\n"
Expand Down Expand Up @@ -392,7 +392,7 @@ sds luaCreateFunction(client *c, robj *body) {
if (luaL_loadbuffer(lctx.lua,funcdef,sdslen(funcdef),"@user_script")) {
if (c != NULL) {
addReplyErrorFormat(c,
"Error compiling script (new function): %s\n",
"Error compiling script (new function): %s",
Copy link
Member Author

@oranagra oranagra Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MeirShpilraien why are the newlines here? (and in the other error below, and similar one in script_lua.c)

Copy link

@MeirShpilraien MeirShpilraien Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here its not needed, above I guess you mean here: https://github.com/redis/redis/pull/10279/files#diff-3f6891fec05432f415cfd49150db67e2bc72d20e0a94a4e3ac762f7ff55f9921R250 ?
If so its a script to you need to specify end of line.

Copy link
Member Author

@oranagra oranagra Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mean that i think you should mirror these 3 changes (trimming excessive newline in 3 places) to your PR (the one that replaces this one)

Copy link

@MeirShpilraien MeirShpilraien Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, will add it.

lua_tostring(lctx.lua,-1));
}
lua_pop(lctx.lua,1);
Expand All @@ -403,8 +403,9 @@ sds luaCreateFunction(client *c, robj *body) {

if (lua_pcall(lctx.lua,0,0,0)) {
if (c != NULL) {
addReplyErrorFormat(c,"Error running script (new function): %s\n",
lua_tostring(lctx.lua,-1));
const char* errstr = lua_tostring(lctx.lua,-1);
addReplyErrorFormat(c,"-%s. Error running script (new function)",
errstr);
}
lua_pop(lctx.lua,1);
return NULL;
Expand Down
2 changes: 1 addition & 1 deletion src/function_lua.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ int luaEngineInitEngine() {
" i = dbg.getinfo(3,'nSl')\n"
" end\n"
" if i then\n"
" return i.source .. ':' .. i.currentline .. ': ' .. err\n"
" return err .. '. ' .. i.source .. ':' .. i.currentline\n"
Copy link
Contributor

@yoav-steinberg yoav-steinberg Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets try to use this PR to unify this with the errh_func script in scriptingInit().

" else\n"
" return err\n"
" end\n"
Expand Down
4 changes: 4 additions & 0 deletions src/networking.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,10 @@ void addReplyErrorLength(client *c, const char *s, size_t len) {

/* Do some actions after an error reply was sent (Log if needed, updates stats, etc.) */
void afterErrorReply(client *c, const char *s, size_t len) {
/* If the script handles the error, it should not be counted, if it'll return the error to the client, then it'll be counted. */
if (c->flags & CLIENT_SCRIPT)
return;

/* Increment the global error counter */
server.stat_total_error_replies++;
/* Increment the error stats
Expand Down
5 changes: 3 additions & 2 deletions src/script_lua.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1419,8 +1419,9 @@ void luaCallFunction(scriptRunCtx* run_ctx, lua_State *lua, robj** keys, size_t
}

if (err) {
addReplyErrorFormat(c,"Error running script (call to %s): %s\n",
run_ctx->funcname, lua_tostring(lua,-1));
const char* errstr = lua_tostring(lua,-1);
addReplyErrorFormat(c,"-%s. Error running script (call to %s)",
errstr, run_ctx->funcname);
lua_pop(lua,1); /* Consume the Lua reply and remove error handler. */
} else {
/* On success convert the Lua return value into Redis protocol, and
Expand Down
48 changes: 37 additions & 11 deletions tests/unit/scripting.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ start_server {tags {"scripting"}} {
r set foo bar
catch {run_script_ro {redis.call('del', KEYS[1]);} 1 foo} e
set e
} {*Write commands are not allowed from read-only scripts*}
} {ERR Write commands are not allowed from read-only scripts*}

if {$is_eval eq 1} {
# script command is only relevant for is_eval Lua
Expand Down Expand Up @@ -439,12 +439,12 @@ start_server {tags {"scripting"}} {
test {Globals protection reading an undeclared global variable} {
catch {run_script {return a} 0} e
set e
} {*ERR*attempted to access * global*}
} {ERR*attempted to access * global*}

test {Globals protection setting an undeclared global*} {
catch {run_script {a=10} 0} e
set e
} {*ERR*attempted to create global*}
} {ERR*attempted to create global*}

test {Test an example script DECR_IF_GT} {
set decr_if_gt {
Expand Down Expand Up @@ -599,8 +599,8 @@ start_server {tags {"scripting"}} {
} {ERR Number of keys can't be negative}

test {Scripts can handle commands with incorrect arity} {
assert_error "*Wrong number of args calling Redis command from script" {run_script "redis.call('set','invalid')" 0}
assert_error "*Wrong number of args calling Redis command from script" {run_script "redis.call('incr')" 0}
assert_error "ERR Wrong number of args calling Redis command from script.*" {run_script "redis.call('set','invalid')" 0}
assert_error "ERR Wrong number of args calling Redis command from script.*" {run_script "redis.call('incr')" 0}
}

test {Correct handling of reused argv (issue #1939)} {
Expand Down Expand Up @@ -723,7 +723,7 @@ start_server {tags {"scripting"}} {
} 0] {}

# Check error due to invalid command
assert_error {ERR *Invalid command passed to redis.acl_check_cmd()} {run_script {
assert_error {Invalid command passed to redis.acl_check_cmd()*} {run_script {
return redis.acl_check_cmd('invalid-cmd','arg')
} 0}
}
Expand Down Expand Up @@ -1288,7 +1288,7 @@ start_server {tags {"scripting"}} {
r config set maxmemory 1

# Fail to execute deny-oom command in OOM condition (backwards compatibility mode without flags)
assert_error {ERR Error running script *OOM command not allowed when used memory > 'maxmemory'.} {
assert_error {OOM command not allowed when used memory > 'maxmemory'.*Error running script*} {
r eval {
redis.call('set','x',1)
return 1
Expand Down Expand Up @@ -1319,7 +1319,7 @@ start_server {tags {"scripting"}} {
}

test "no-writes shebang flag" {
assert_error {ERR Error running script *Write commands are not allowed from read-only scripts.} {
assert_error {ERR Write commands are not allowed from read-only scripts.*Error running script*} {
r eval {#!lua flags=no-writes
redis.call('set','x',1)
return 1
Expand Down Expand Up @@ -1404,12 +1404,16 @@ start_server {tags {"scripting"}} {
# Additional eval only tests
start_server {tags {"scripting"}} {
test "Consistent eval error reporting" {
r config resetstat
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'.} {
assert_error {OOM command not allowed when used memory > 'maxmemory'.*Error running script (call to *)} {
r eval {return redis.call('set','x','y')} 1 x
}
assert_equal [errorrstat OOM r] {count=1}

# redis.pcall() failure due to Redis state (OOM) returns lua error table with Redis error message without '-' prefix
r config resetstat
assert_equal [
r eval {
local t = redis.pcall('set','x','y')
Expand All @@ -1420,16 +1424,27 @@ start_server {tags {"scripting"}} {
end
} 1 x
] 1
# error stats were not incremented
assert_equal [errorrstat ERR r] {}
assert_equal [errorrstat OOM r] {}

# Returning an error object from lua is handled as a valid RESP error result.
r config resetstat
assert_error {OOM command not allowed when used memory > 'maxmemory'.} {
r eval { return redis.pcall('set','x','y') } 1 x
}
assert_equal [errorrstat OOM r] {count=1}

r config set maxmemory 0
r config resetstat
# 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} {
assert_error {ERR DB index is out of range.*Error running script (call to *)} {
r eval {return redis.call('select',99)} 0
}
assert_equal [errorrstat ERR r] {count=1}

# redis.pcall() failure due to error in Redis command returns lua error table with redis error message without '-' prefix
r config resetstat
assert_equal [
r eval {
local t = redis.pcall('select',99)
Expand All @@ -1440,11 +1455,17 @@ start_server {tags {"scripting"}} {
end
} 0
] 1
assert_equal [errorrstat ERR r] {} ;# error stats were not incremented

# 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 config resetstat
assert_error {ERR Write commands are not allowed from read-only scripts.*Error running script (call to *)} {
r eval_ro {return redis.call('set','x','y')} 1 x
}
assert_equal [errorrstat ERR r] {count=1}

# redis.pcall() failure due to scripting specific error state (write cmd with eval_ro) returns lua error table with Redis error message without '-' prefix
r config resetstat
assert_equal [
r eval_ro {
local t = redis.pcall('set','x','y')
Expand All @@ -1455,19 +1476,24 @@ start_server {tags {"scripting"}} {
end
} 1 x
] 1
assert_equal [errorrstat ERR r] {} ;# error stats were not incremented
} {} {cluster:skip}

test "LUA redis.error_reply API" {
r config resetstat
assert_error {MY_ERR_CODE custom msg} {
r eval {return redis.error_reply("MY_ERR_CODE custom msg")} 0
}
assert_equal [errorrstat MY_ERR_CODE r] {count=1}
}

test "LUA redis.status_reply API" {
r config resetstat
r readraw 1
assert_equal [
r eval {return redis.status_reply("MY_OK_CODE custom msg")} 0
] {+MY_OK_CODE custom msg}
assert_equal [errorrstat MY_ERR_CODE r] {} ;# error stats were not incremented
r readraw 0
}
}
Expand Down