Fix memory leak in Redis by using auto memory management #4054
Conversation
| // Only use commands that have auto memory management. | ||
| // TODO(swang): Ideally, we would make the commands that don't have auto memory | ||
| // management inaccessible instead of just using separate namespaces. | ||
| using namespace auto_memory_redis_commands; |
There was a problem hiding this comment.
how about we get rid of this, just have the redis_commands namespace that hides the non-auto ones, and put the others into the global namespace? That would be a bit simpler and would make the auto ones more "canonical".
There was a problem hiding this comment.
we could even call the redis_command namespace something else, like internal etc. to make clear it is "hidden"
There was a problem hiding this comment.
Ah good idea, thanks! Now I'm not sure why I started with two separate namespaces :)
|
This is great, good find :) |
|
Nice! We should (in a separate PR) somehow get this issue to show up in the stress tests, perhaps just by running one of the test workloads in a low-memory node. |
stephanie-wang
force-pushed
the
redis-auto-mem
branch
from
faeb0e9 to
d5db5bd
Compare
|
Test PASSed. |
|
Test FAILed. |
What do these changes do?
We normally use
RedisModule_AutoMemoryto turn on Redis' built-in automatic memory management, but we forgot to enable it forRAY.TABLE_APPEND. Running this command repeatedly will cause Redis to evict all keys and use ever increasing memory.This PR enables automatic memory management for this command and attempts to restructure the Redis module to make explicit which Redis commands have the feature enabled.
Related issue number
I believe this should close #3884.