@Axlgrep thanks for this PR.
there are possibly 3 separate issues here, each with maybe a different solution or considerations:

1. Lua heap should use jemalloc (simply because it's better), no arguments here (*see activedefrag below).
2. Lua heap should (or should not) use zmalloc (account in used_memory)
3. Lua heap should be limited in some way or induce key eviction?

There was at least one previous attempt to handle one of these problems:
#2505
note that it also provide an allocator callback without adding changes in the Lua library code (no merge conflicts if / when it's upgraded).

My problem with including this memory in used_memory is that this is not "evitable" memory, so when Lua scripts eat memory, the right thing to do is force them to stop or do GC, evicting keys won't help (will not prevent a malicious script from eating all the memory on the machine).

on the other hand we can't just cause a running script to stop (specifically if it already performed some write commands), this will break the atomic nature of the script and leave the server in a bad sate (maybe the only thing we can do against such a script is exit(1), i.e. data loss).

I'm not sure how to proceed.

If the PR was just replacing the allocator to jemalloc (bypassing zmalloc), i can easily merge it (although it does damage the effort of activedefrag to understand what's the "defraggable" fragmentation, and could cause it to run forever without achieving anything).

But about the eviction, i'm not sure... especially considering that you're not even solving the problem of a malicious script (it can keep running and eat memory and the eviction won't help, or even be triggered until the script exits).

@redis/core-team please share your thoughts.

@oranagra First of all, thank you for your patient answer, I previously ignored eval/evalsha/script commands without use-memory flags, which would result in these command being able to continue executing even after the maxmemory has been reached, now I've added it.

I just want to control the upper limit of single Redis memory to avoid it eating all the memory on the machine.

@Axlgrep sadly, you can't just mark the EVAL* commands a as use-memory, what about read only scripts? these should be able to run even if the server is over the memory (like other read only commands).
But regardless, it doesn't solve the problem, since a script that's running redis commands will pass that check (before the script), and then continue to eat more and more memory (if the script runs redis commands in a loop).

This use-memory flag refers to redis (evictable memory), not the Lua heap (which is / was the topic of this PR).
There is another related topic, which is that redis commands which are executed from within the script don't trigger eviction, this was done to resolve replication inconsistency issues back when scripts where replicated as scripts rather than replicating the commands they call.
Anyway, this is for another discussion. You showed that the Lua heap is what's eating your memory, not the database due to missing eviction. So let's drop these changes and get back to discuss Lua heap.

The above goes to show that a malicious script can run in a loop and eat redis (keys) memory forever. There's nothing to do once it generated its first write command except for terminating the server (killing the script would leave the partial database modifications it made and would violate atomicity).

So we should decide what we want to focus on, protecting the host from malicious scripts? Or improving Lua heap handling?

P.s. There's absolutely no reason to mark the SCRIPT command as use-memory (the only effect of that is to prevent SCRIPT KILL from being used)

@oranagra Lua consume memory on the one hand is consume by Lua heap, on the other hand we cache Lua script in server.lua_scripts, it also consumes memory(Eval and Script Load command can lead to increased memory risk, even if read only scripts).

If do not add the use-memory flag to these commands, when user loads a large number of Lua scripts with malicious intent, used_memory will not be limited, as shown in the illustration below,
maxmemory_human is 4GB, but used_memory_human reach 10.33GB, There may even be more.

Script does have many subcommands, to avoid being unable to execute the Script kill command after the maxmemory is reached, I can clear the use-memory flags in Script command. however, need to do something special with the Scirpt Load command , as follows?

finally, I don't have a good solution to the memory increase you mentioned due to the internal loop of the Lua script.

I forgot about script cache consuming Lua heap, but i feel that this is another example of why we don't want to count the scripts as part of the normal used_memory and have them induce key eviction while the cache itself is unlimited. instead i think we need to "evict" old scripts from the script cache.
i.e. count the script cache memory separately and have some limit for it, when it goes over the limit, delete old scripts. i think this would be better than blocking new scripts and responding with error to user commands.

IMO it may be ok to also count the Lua heap in used_memory too, so that if there are no scripts the keys can use the whole maxmemory, and if there are scripts, there's a little less memory for keys, but still the scripts should be "evictable" and have their own limit, so that Lua can't cause eviction of all keys, and at the same time be unlimited (no mechanism to remove scripts from the cache or limit the Lua heap).

I feel that we may want to split this PR / discussion to several topics so that it's easier to reach a decision and a design for each.

I guess these are:

1. Lua script cache limit and removal of old scripts.
2. Lua heap / GC is limited (heap leftovers between script executions, what new Lua memory limit #2505 attempted to solve).
3. Lua uses zmalloc or just jemalloc.
4. Long running malicious scripts and their abuse of redis commands.
5. Long running malicious scripts and their abuse of Lua heap.

The last two topics are a serious problem, but i don't see any way out except exit(1).
Maybe you wanna start by tackling the first topic?

We discussed this topic in a core team meeting.
We would like to try to address Lua memory like we treat client output buffers see #7676

This means the following:

1. Lua uses zmalloc and visible in used_memory
2. have a configurable memory limit for Lua scripts, when below the limit, they can cause some key eviction, but above it we try to limit their used memory and prevent key eviction. We do that by forcing GC on the Lua VM, and evicting old script code (see below).
3. active defrag should probably just trigger Lua GC before computing the fragmentation ratio and defrag effort.
4. we need to look into a way to remove old scripts from the cache when there are many scripts and their code is eating a lot of memory. this does break the contract with the client in some way (client will be forced to reload a script if EVALSHA fails), such a change probably needs to be done in a major version release.

The above are related to the script memory usage in between script execution (and it's effect on keys eviction), and should be done all at once (i.e can be several PRs, but these should make it into one redis release)

besides that we also want to stop malicious scripts from eating too much memory during a single script execution.

1. watch the memory usage of redis (consumed by redis commands executed by the script, which do not do key eviction), and when goes over some threshold (200% or 110% of memory limit?), try to abort the script.
2. watch the Lua heap memory used by the script and above a certain threshold try to abort the script.
3. aborting a script may not be possible when it already performed modifications on the database, in which case we need to exit(1).

@oranagra ,
Regarding bullet 2, "have a configurable memory limit for Lua scripts".

A naive option is to add a threshold for the max memory usage by Lua scripts. Once the usage exceed the threshold abort creation of new scripts. As you said the main reason of not removing old scripts in some LRU fashion is to keep the contract.

In order to follow the contract Redis shall abort the script creation only if the following conditions are met:

1. The memory usage by Lua scripts is above the threshold.
2. The creation is not part of a PIPELINE. Redis user can assume (according the Redis documentation) that the load script command cannot fail, that is, the consecutive EVALSHA commands will find the SHA. For more info see redis documentation references at the Redis documentation section.
3. The node role is MASTER (not REPLICA). The main reason is to keep the state consistent.

I also added configuration for enforcing the limit, in the common case we just want to alert to the user and not enforce the limit. The user can either configure the limit or monitor the usage statistics.

The attached code contains:

1. The threshold enforcement.
2. New statistics for the number of "rejected" scripts.
3. TCL tests.

If you find it relevant, please review the attached code, the relevant part is in scripting.c at the function luaCreateFunction.
The following code contains a git diff of the proposal:
LuaLimit.log

Regarding the contract, few comments from the redis documentation.

1. A note on the expected amount of scripts - copied from https://redis.io/commands/eval

"""

The reason why scripts can be cached for long time is that it is unlikely for a well written application to have enough different scripts to cause memory problems. Every script is conceptually like the implementation of a new command, and even a large application will likely have just a few hundred of them. Even if the application is modified many times and scripts will change, the memory used is negligible.

"""

2. Two references about the fact that EVALSHA shall not fail when it is used in a pipeline

a. Copied from https://redis.io/commands/eval

"""

_The fact that the user can count on Redis not removing scripts is semantically useful in the context of pipelining.

For instance an application with a persistent connection to Redis can be sure that if a script was sent once it is still in memory, so EVALSHA can be used against those scripts in a pipeline without the chance of an error being generated due to an unknown script (we'll see this problem in detail later).

A common pattern is to call SCRIPT LOAD to load all the scripts that will appear in a pipeline, then use EVALSHA directly inside the pipeline without any need to check for errors resulting from the script hash not being recognized._

"""

b. Copied from https://redis.io/topics/pipelining

"""

Sometimes the application may also want to send EVAL or EVALSHA commands in a pipeline. This is entirely possible and Redis explicitly supports it with the SCRIPT LOAD command (it guarantees that EVALSHA can be called without the risk of failing).

@asafpor thanks for all docs references and patch.
Here's my initial feedback:

1. you seem to be confusing between a transaction (MULTI) and a pipeline. Client can indeed use SCRIPT LOAD and EVALSHA in a pipeline (writing them to the socket without waiting for responses and expect the script to always be found), and that's what the documentation refers to. however, there's no good way for redis to tell if pipeline is used or not. so in that sense, your patch can still cause the application to fail in an unexpected way.
2. i think we must to find a way to evict scripts from the cache rather than block new ones (even if it means a change of the contract with the client). a change like what you did would mean that a badly written application (which is the focus of this discussion) will initially work, and eventually stop working (when the limit is reached), if however we implement some kind of LRU eviction for scripts, these applications are likely to keep working forever.
3. when counting the memory, we'll probably want to include the compiled script code (allocations made by the Lua VM)

@oranagra ,

1. Thanks for the clarification regarding the pipeline and MULTI.
2. Auto eviction of scripts requires updating all clients of Redis to handle the new case of failures of EVALSHA. You will probably have to add a new eviction policy configuration for LUA scripts in order to be backward compatible and keep the default same as today.
3. The approach I suggested is very simple and is orthogonal to script eviction policy. I agree that there are still issues to solve and my solution apparently break the contract as well for pipeline.

Thanks for your feedback.

I also meet the same problem recently. The user abuses the script (putting the parameter format into the lua body) and consumes a lot of memory (and keeps consuming it).

@oranagra From the current perspective, is there anything else that i can do? i can do some work since i have some free time and like to working on it.

Lua script cache limit and removal of old scripts.

since we do have a luaScript and the lua body is a robj, we can put lru in like lookupkey, and then do the same thing in evict.

other:
Note that even if we use script flush async, when the lua heap is huge, it will still block the main thread.

@enjoy-binbin i'm not sure how to proceed at the present time. script eviction could be a breaking change, so unless we can somehow make it safe, it'll be hard to make a decision on that.
maybe there's a way to make the SCRIPT FLUSH truly async, or add another SCRIPT FLUSHCACHE that will gradually delete just the scripts from the VM in the background, without creating a fresh VM.

I believe we can make SCRIPT FLUSH run truly async. We can take the current lua instance on lctx.lua and call lua_close on it in a background thread. On the main thread we just replace it with a new Lua instance (as we already do). I believe this should work.

I believe we can make SCRIPT FLUSH run truly async. We can take the current lua instance on lctx.lua and call lua_close on it in a background thread. On the main thread we just replace it with a new Lua instance (as we already do). I believe this should work.

great, i can take a try (I still have the context). it look solid, thanks

I also meet the same problem recently. The user abuses the script (putting the parameter format into the lua body) and consumes a lot of memory (and keeps consuming it).

@oranagra Almost all of these cases I have seen come from misuse of EVAL and not SCRIPT LOAD + EVALSHA. If we could figure out how to just evict scripts generated from EVAL, I do think it would go aways to solve this specific problem. This wouldn't immediately involve a breaking change.

interesting idea.
You are proposing to keep a record to know which scripts were generated by SCRIPT LOAD, and which ones by EVAL, and only evict the ones generated by EVAL.
In theory, a user can maybe load a script with EVAL and then use EVALSHA to call it (by calculating the SHA1 value on the client side), it could be that if we read the docs carefully we'll realized it's a valid scenario, but i suppose it's extremely rare.

I support that idea.
@enjoy-binbin do you wanna try to draft a PR for that?

p.s. the only real downside in my eyes is that users that abuse EVAL generating a new script on each call, will get away with it, and won't ever know they're doing something wrong. but that's obliviously not an argument, there are many other things you can do wrong and get away with it.

good, i am willing to take a try. So we will add a configuration item similar to lua-maxmemory. Then separate the eval and script load scripts, and in evict we will only evicts eval scripts, evict will using the same maxmemory-policy (or a new lua-maxmemory-policy)

i think it can be a plain LRU, i don't think the eviction policy needs a config.
i wouldn't mind re-using the other config, but it also has a noevictoin option, which i don't want to support.

arguably, if the LRU eviction mechanism becomes complicated, we can even just flush all these scripts when hitting the limit (won't be a breaking change). but this could cause performance overhead if one script is repeatedly used (with EVAL), and others are just one time scripts, the re-used one would require re-compilation.

p.s. the only real downside in my eyes is that users that abuse EVAL generating a new script on each call, will get away with it, and won't ever know they're doing something wrong. but that's obliviously not an argument, there are many other things you can do wrong and get away with it.

I suppose we can always add an info field so they know.

i got tired of seeing this closed issue being tracked in the projects as ToDo. created a new one with summary of the ideas left to handle #13102