Skip to content

Unified Lua and modules reply parsing and added resp3 support to RM_Call#9202

Merged
oranagra merged 56 commits intoredis:unstablefrom
MeirShpilraien:unified_module_and_lua_resp_parsing
Aug 4, 2021
Merged

Unified Lua and modules reply parsing and added resp3 support to RM_Call#9202
oranagra merged 56 commits intoredis:unstablefrom
MeirShpilraien:unified_module_and_lua_resp_parsing

Conversation

@MeirShpilraien
Copy link

@MeirShpilraien MeirShpilraien commented Jul 6, 2021

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 (#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.

TODO:

  • Parse resp3 attribute and add it to Lua and modules
  • Document Lua big number and verbatim reply format

Left for future PR's:

  • Parse push notifications - need to be decided if it's relevant for module/Lua, for now, we are not implementing it

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.
@MeirShpilraien MeirShpilraien requested a review from oranagra July 6, 2021 06:54
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I didn't look very carefully but I have two comments:

  • Documentation in Redis is in the .c files, not in the .h files.
  • 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.

@MeirShpilraien
Copy link
Author

Thanks @zuiderkwast

Documentation in Redis is in the .c files, not in the .h files.

Sure, will change that

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.

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 :)

@oranagra
Copy link
Member

oranagra commented Jul 7, 2021

@MeirShpilraien please improve the top comment to include the intensive for doing this PR (share code between Lua, Modules, and the future Functions).
and if you can, elaborate a bit more on what you did in this PR, i.e. the exact roles of the new units, and any other change you did, so that it'll be easier to review.

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?

Meir Shpilraien (Spielrein) and others added 8 commits July 12, 2021 18:22
@oranagra
Copy link
Member

oranagra commented Jul 22, 2021

@redis/core-team please approve the new API (till now we were focused on the refactoring and doc/comments).
Please see the top comment for the full list.
it's basically 3 things:

  1. new module APIs for parsing RESP3 responses that come back from RM_Calll
  2. new arguments for RM_Call: 3 for RESP3, and 0 for auto
  3. new flag for RM_GetContextFlags for detecting the protocol of the current client
  4. new lua capabilities for parsing RESP3 features unused till now (bignum, verbatim, attributes)

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

@oranagra oranagra added the state:major-decision Requires core team consensus label Jul 22, 2021
@oranagra oranagra mentioned this pull request Jul 26, 2021
52 tasks
yossigo
yossigo previously approved these changes Aug 2, 2021
oranagra
oranagra previously approved these changes Aug 2, 2021
madolson
madolson previously approved these changes Aug 2, 2021
Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API looks good to me.

@MeirShpilraien MeirShpilraien dismissed stale reviews from madolson, oranagra, and yossigo via f66dd94 August 3, 2021 13:59
@oranagra oranagra merged commit 2237131 into redis:unstable Aug 4, 2021
oranagra pushed a commit that referenced this pull request Aug 4, 2021
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.
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
…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]>
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
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.
oranagra pushed a commit that referenced this pull request Sep 9, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants