Redis Stack allocated RedisModuleString

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 RedisModuleString whether 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-allocated RedisModuleString but not both. Unfortunately, there are functions that might return both in certain situations.
Introduce a new function that gets RedisModuleString and return a copy of it. If the RedisModuleString is 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.

Comment From: oranagra

i support option 3. would like to get feedback from @yoav-steinberg @guybe7 @yossigo. note: where the assertion was introduced here #3243 (see discussion), few recent issues with it are: #7536, #7528

Comment From: guybe7

i vote for 3.

Comment From: yossigo

@MeirShpilraien I also like option 3, but it needs to be clear for callers that the resulting string is still not a real duplicate and should be immutable. In the past I tried to get the Module API to be more const-correct but there's still a long way to go.

Comment From: oranagra

ohh, I wasn't aware of RM_StringAppendBuffer, thinking that strings are always immutable. seems that it's the only one that can change an existing string, but note that it returns NULL in case the refcount was already greater than 1.

Comment From: MeirShpilraien

Yes, @oranagra it returns REDISMODULE_ERR so we should be good.

Comment From: yoav-steinberg

Option 3.

Comment From: MeirShpilraien

If we all agree I can make a PR. Let me know whether of not to proceed.

Comment From: oranagra

i think we can proceed. but coding it is the easy part, choosing a name and writing the docs is the hard part.

The naming convention is a mess. guess which of these work on string and which work on keys: - StringAppendBuffer - RetainString - StringCompare - StringTruncate - StringPtrLen - FreeString

The bad options i see: - RM_RetainString2 - RM_RetainStringOrCopy - RM_HoldString - RM_KeepString - RM_StringGetCopy - RM_IsStringRetainable (indicating the user should call RM_CreateStringFromString instead)

Comment From: MeirShpilraien

@oranagra I would go with RM_KeepString (or maybe RM_StringKeep ?). Also how about giving an optional flag for no copy, in this case, if the flag is on and the RMString is stack allocated it will return NULL (though I am not sure anyone will use it). Another thing that we can do is add an indication of whether or not the String was copied (with an output variable for example). Again not sure if anyone will use it.

Of course, we can wrap it all with macros for defaults and make it easier to use

Let me know what you think.

Comment From: oranagra

I don't think we want another boolean input to the API, I think for this case we just wanna tell the user, if you need to keep that string, call that API and keep the return value rather than the input.

considering we wanna deprecate RM_RetainString and ask everyone to use this one, i guess RM_KeepString is a good name, but let's wait for more input.

to check if it was copied, the user can just compare the input and output pointers.

Comment From: yossigo

Nitpicking: RM_HoldString() is more clear to me. And I think that as part of this PR we should also add a deprecation note (i.e. compiler deprecate attribute) to RM_RetainString() to get the clock ticking.

Comment From: oranagra

@yossigo i'm not so sure about the deprecation i think there are use cases where the user can't change the pointer he's holding, and would rather keep using Retain which doesn't have a return value. also, considering the echo system not sure we wanna start deprecating things. and even if we do, maybe just after the new API gains some millage and we're more confident that there no no other holes.

Comment From: yossigo

@oranagra But this promotes a leaky abstraction, you count on the user knowing in what scenarios strings are not stack based so RM_RetainString() can succeeds, and this becomes an informal but binding API contract.

From my POV, ideally, RM_RetainString() should not be used for any new code and users should refactor any use that assumes the pointer can be retained.

Comment From: oranagra

@yossigo what about modules that need to be compatible with several redis versions? on one hand they can't use RM_HoldString since that would make them incompatible with the older versions, but on other hand they'll get annoying warnings when being built with thew new header (which they might need in order to conditionally use some of the newer features)?

Comment From: yossigo

@oranagra that's a bigger issue around deprecation of module API. I think this is anyway not the time to discuss that, maybe revisit this issue before 7.0 (and postpone any deprecation notices until then).