Unified Lua and modules reply parsing and added resp3 support to RM_Call#9202
Conversation
The PR introduces 2 new units: * call_reply: wraps the RedisModuleCallReply object * reply_parser: handle parsing a reply generated by command execution (either by Lua or RM_Call) The call_reply unit takes a reply buffer and uses the parser to parse it and create a CallReply object. RM_Call uses this unit to create and interact with the CallReply. Lua uses directly the reply_parser unit to parse a reply buffer and create the Lua object. The parser is able to pars resp3, so this gives us the ability to use resp3 with RM_Call. Tests were added to verify this capability.
zuiderkwast
left a comment
There was a problem hiding this comment.
Interesting. I didn't look very carefully but I have two comments:
- Documentation in Redis is in the
.cfiles, not in the.hfiles. - Hiredis already contains a reply parser. It's a simple protocol, so maybe there is not problem to have one more parser, but it can be considered if we want to improve/refactor/export the one in hiredis instead.
|
Thanks @zuiderkwast
Sure, will change that
The hiredis parser checks for protocol errors, here the parser assumes there are no protocol errors (because the reply was generated by Redis itself) and it can be faster. and in general, this PR is not adding one more parser, its actually removes one extra parser :) |
|
@MeirShpilraien please improve the top comment to include the intensive for doing this PR (share code between Lua, Modules, and the future Functions). p.s. you say "call_reply wraps the RedisModuleCallReply object" but isn't it the other way around? or actually that it replaces (renames) it? |
The new debug subcommand was added so it will be possible to run 'debug protocol' from scripts and test resp3 parsing. Lua resp3 tests was moved back to scripting.c
Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: Oran Agra <[email protected]>
|
@redis/core-team please approve the new API (till now we were focused on the refactoring and doc/comments).
some of these will be needed if RM_Call and Lua will call module functions that use RESP3 features that redis doesn't yet yet (attributes, bignum), i.e. #8521 |
f66dd94
Fix test introduced in #9202 that failed on 32bit CI. The failure was due to a wrong double comparison. Change code to stringify the double first and then compare.
…all (redis#9202) ## Current state 1. Lua has its own parser that handles parsing `reds.call` replies and translates them to Lua objects that can be used by the user Lua code. The parser partially handles resp3 (missing big number, verbatim, attribute, ...) 2. Modules have their own parser that handles parsing `RM_Call` replies and translates them to RedisModuleCallReply objects. The parser does not support resp3. In addition, in the future, we want to add Redis Function (redis#8693) that will probably support more languages. At some point maintaining so many parsers will stop scaling (bug fixes and protocol changes will need to be applied on all of them). We will probably end up with different parsers that support different parts of the resp protocol (like we already have today with Lua and modules) ## PR Changes This PR attempt to unified the reply parsing of Lua and modules (and in the future Redis Function) by introducing a new parser unit (`resp_parser.c`). The new parser handles parsing the reply and calls different callbacks to allow the users (another unit that uses the parser, i.e, Lua, modules, or Redis Function) to analyze the reply. ### Lua API Additions The code that handles reply parsing on `scripting.c` was removed. Instead, it uses the resp_parser to parse and create a Lua object out of the reply. As mentioned above the Lua parser did not handle parsing big numbers, verbatim, and attribute. The new parser can handle those and so Lua also gets it for free. Those are translated to Lua objects in the following way: 1. Big Number - Lua table `{'big_number':'<str representation for big number>'}` 2. Verbatim - Lua table `{'verbatim_string':{'format':'<verbatim format>', 'string':'<verbatim string value>'}}` 3. Attribute - currently ignored and not expose to the Lua parser, another issue will be open to decide how to expose it. Tests were added to check resp3 reply parsing on Lua ### Modules API Additions The reply parsing code on `module.c` was also removed and the new resp_parser is used instead. In addition, the RedisModuleCallReply was also extracted to a separate unit located on `call_reply.c` (in the future, this unit will also be used by Redis Function). A nice side effect of unified parsing is that modules now also support resp3. Resp3 can be enabled by giving `3` as a parameter to the fmt argument of `RM_Call`. It is also possible to give `0`, which will indicate an auto mode. i.e, Redis will automatically chose the reply protocol base on the current client set on the RedisModuleCtx (this mode will mostly be used when the module want to pass the reply to the client as is). In addition, the following RedisModuleAPI were added to allow analyzing resp3 replies: * New RedisModuleCallReply types: * `REDISMODULE_REPLY_MAP` * `REDISMODULE_REPLY_SET` * `REDISMODULE_REPLY_BOOL` * `REDISMODULE_REPLY_DOUBLE` * `REDISMODULE_REPLY_BIG_NUMBER` * `REDISMODULE_REPLY_VERBATIM_STRING` * `REDISMODULE_REPLY_ATTRIBUTE` * New RedisModuleAPI: * `RedisModule_CallReplyDouble` - getting double value from resp3 double reply * `RedisModule_CallReplyBool` - getting boolean value from resp3 boolean reply * `RedisModule_CallReplyBigNumber` - getting big number value from resp3 big number reply * `RedisModule_CallReplyVerbatim` - getting format and value from resp3 verbatim reply * `RedisModule_CallReplySetElement` - getting element from resp3 set reply * `RedisModule_CallReplyMapElement` - getting key and value from resp3 map reply * `RedisModule_CallReplyAttribute` - getting a reply attribute * `RedisModule_CallReplyAttributeElement` - getting key and value from resp3 attribute reply * New context flags: * `REDISMODULE_CTX_FLAGS_RESP3` - indicate that the client is using resp3 Tests were added to check the new RedisModuleAPI ### Modules API Changes * RM_ReplyWithCallReply might return REDISMODULE_ERR if the given CallReply is in resp3 but the client expects resp2. This is not a breaking change because in order to get a resp3 CallReply one needs to specifically specify `3` as a parameter to the fmt argument of `RM_Call` (as mentioned above). Tests were added to check this change ### More small Additions * Added `debug set-disable-deny-scripts` that allows to turn on and off the commands no-script flag protection. This is used by the Lua resp3 tests so it will be possible to run `debug protocol` and check the resp3 parsing code. Co-authored-by: Oran Agra <[email protected]> Co-authored-by: Yossi Gottlieb <[email protected]>
Fix test introduced in redis#9202 that failed on 32bit CI. The failure was due to a wrong double comparison. Change code to stringify the double first and then compare.
When parsing an array type reply, ctx will be lost when recursively parsing its elements, which will cause a memory leak in automemory mode. This is a result of the changes in #9202 Add test for callReplyParseCollection fix
Current state
reds.callreplies and translates them to Lua objects that can be used by the user Lua code. The parser partially handles resp3 (missing big number, verbatim, attribute, ...)RM_Callreplies and translates them to RedisModuleCallReply objects. The parser does not support resp3.In addition, in the future, we want to add Redis Function (#8693) that will probably support more languages. At some point maintaining so many parsers will stop scaling (bug fixes and protocol changes will need to be applied on all of them). We will probably end up with different parsers that support different parts of the resp protocol (like we already have today with Lua and modules)
PR Changes
This PR attempt to unified the reply parsing of Lua and modules (and in the future Redis Function) by introducing a new parser unit (
resp_parser.c). The new parser handles parsing the reply and calls different callbacks to allow the users (another unit that uses the parser, i.e, Lua, modules, or Redis Function) to analyze the reply.Lua API Additions
The code that handles reply parsing on
scripting.cwas removed. Instead, it uses the resp_parser to parse and create a Lua object out of the reply. As mentioned above the Lua parser did not handle parsing big numbers, verbatim, and attribute. The new parser can handle those and so Lua also gets it for free. Those are translated to Lua objects in the following way:{'big_number':'<str representation for big number>'}{'verbatim_string':{'format':'<verbatim format>', 'string':'<verbatim string value>'}}Tests were added to check resp3 reply parsing on Lua
Modules API Additions
The reply parsing code on
module.cwas also removed and the new resp_parser is used instead. In addition, the RedisModuleCallReply was also extracted to a separate unit located oncall_reply.c(in the future, this unit will also be used by Redis Function). A nice side effect of unified parsing is that modules now also support resp3. Resp3 can be enabled by giving3as a parameter to the fmt argument ofRM_Call. It is also possible to give0, which will indicate an auto mode. i.e, Redis will automatically chose the reply protocol base on the current client set on the RedisModuleCtx (this mode will mostly be used when the module want to pass the reply to the client as is). In addition, the following RedisModuleAPI were added to allow analyzing resp3 replies:New RedisModuleCallReply types:
REDISMODULE_REPLY_MAPREDISMODULE_REPLY_SETREDISMODULE_REPLY_BOOLREDISMODULE_REPLY_DOUBLEREDISMODULE_REPLY_BIG_NUMBERREDISMODULE_REPLY_VERBATIM_STRINGREDISMODULE_REPLY_ATTRIBUTENew RedisModuleAPI:
RedisModule_CallReplyDouble- getting double value from resp3 double replyRedisModule_CallReplyBool- getting boolean value from resp3 boolean replyRedisModule_CallReplyBigNumber- getting big number value from resp3 big number replyRedisModule_CallReplyVerbatim- getting format and value from resp3 verbatim replyRedisModule_CallReplySetElement- getting element from resp3 set replyRedisModule_CallReplyMapElement- getting key and value from resp3 map replyRedisModule_CallReplyAttribute- getting a reply attributeRedisModule_CallReplyAttributeElement- getting key and value from resp3 attribute replyNew context flags:
REDISMODULE_CTX_FLAGS_RESP3- indicate that the client is using resp3Tests were added to check the new RedisModuleAPI
Modules API Changes
3as a parameter to the fmt argument ofRM_Call(as mentioned above).Tests were added to check this change
More small Additions
debug set-disable-deny-scriptsthat allows to turn on and off the commands no-script flag protection. This is used by the Lua resp3 tests so it will be possible to rundebug protocoland check the resp3 parsing code.TODO:
Left for future PR's: