[BUG] 'EVAL does not leak in the Lua stack' (scripting.tcl) seems to be a flakey test

[sjpotter]

Describe the bug

the existing code for this test is

test {EVAL does not leak in the Lua stack} {
    r set x 0
    # Use a non blocking client to speedup the loop.
    set rd [redis_deferring_client]
    for {set j 0} {$j < 10000} {incr j} {
        run_script_on_connection $rd {return redis.call("incr",KEYS[1])} 1 x
    }
    for {set j 0} {$j < 10000} {incr j} {
        $rd read
    }
    assert {[s used_memory_lua] < 1024*100}
    $rd close
    r get x
} {10000}

but I think that's flakey against, a long running redis instance i have that I've been testing against.

to see this, I changed it to

test {EVAL does not leak in the Lua stack} {
    set preused [s used_memory_lua]
    assert {$preused < 1024*100}
    r set x 0
    # Use a non blocking client to speedup the loop.
    set rd [redis_deferring_client]
    for {set j 0} {$j < 10000} {incr j} {
        run_script_on_connection $rd {return redis.call("incr",KEYS[1])} 1 x
    }
    for {set j 0} {$j < 10000} {incr j} {
        $rd read
    }
    set used [s used_memory_lua]
    assert {$used < 1024*100}
    $rd close
    r get x
} {10000}

and it asserts right t the beginning, i.e. it enters the test having "leaked" memory.

But if I remove the first assert and change the second assert to something like

assert {($used - $preused) == 0}, it can show that memory actually decreases over the course of this test (i.e. used < $preused, but not equal to each other).

so this test (or entire suite) seems flakey when scripting.tcl is run repeatedly against a long running redis.

A short description of the bug.

test fails under repeated runs against a redis instance

To reproduce

compiled redis with sanitizer, repeatedly run

./runtest --host 127.0.0.1 --port 5001 --cluster-mode --singledb --ignore-encoding --ignore-digest --skipfile /home/spotter/CLionProjects/redisraft/tests/redis-suite/skip.txt --tags -needs:repl --tags -needs:debug --tags -needs:save --tags -needs:reset --tags -needs:config-maxmemory --tags -needs:latency --tags -stream --tags -pause --tags -tracking --tags -cli --tags -querybuf --single unit/scripting

(not sure all those flags are needed to reproduce, just happen to be how I'm testing at the moment)

Expected behavior

not to succeed first (possibly few) times but then reliably begin to fail

[sjpotter]

tested against 3c4def5

[oranagra]

i think the test needs to start with SCRIPT RESET or maybe be added an external:skip flag.
@MeirShpilraien WDYT?

[MeirShpilraien]

Yes agree, either this or we can check the memory usage before and after and only check the threshold (like @sjpotter did in the example he gave).

[oranagra]

since Lua can GC at some point, i'm not sure that approach is valid.
i.e. a case where we had high memory utilization at the beginning, and it all got wiped up later, and we won't detect the leak.
am i wrong?

[MeirShpilraien]

Yes, good point, so probably SCRIPT RESET is the best option.