-
Notifications
You must be signed in to change notification settings - Fork 24.5k
Description
I notice that there are some locations where the robj pass to a module callback as a RedisModuleString might be stack-allocated. This prevents the module writer from using RedisModule_RetainString on the given RedisModuleString, if the module writer will try to retain the given RedisModuleString he will crash on the assert.
While I believe its not a big production issue (the module writer will identify it on the testing stage probably). It might be inconvenient for the module writer because there is no way to know on the development stage if the given RedisModuleString is stack-allocated or not. In addition, there might be some rare code paths that are not tested and retain a stack-allocated RedisModuleString, those code paths might crash on production (totally the module writer fault of not testing his code but still it's confusing).
I can see 3 options to handle it:
- Document each function that receives a
RedisModuleStringwhether or not it's ok to retain the given it. - Introduce a new struct that will present the stack-allocated
RedisModuleString. Notice that this option is good only if a function always returns either stack-allocated or heap-allocatedRedisModuleStringbut not both. Unfortunately, there are functions that might return both in certain situations. - Introduce a new function that gets
RedisModuleStringand return a copy of it. If theRedisModuleStringis heap-allocated then it just increases the ref count and returns the same object. If the given RedisModuleString is stack-allocated then it creates a copy on the heap and returns it.
In my opinion, option 3 is the best. I believe that introducing such function and encourage module writers to use it instead of RedisModule_RetainString will allow, eventually, to deprecate RedisModule_RetainString.
Would like to hear your thoughts and opinions.