- 04 Aug, 2021 1 commit
-
-
Meir Shpilraien (Spielrein) authored
## 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. Co-authored-by:
Oran Agra <oran@redislabs.com> Co-authored-by:
Yossi Gottlieb <yossigo@gmail.com>
-
- 03 Aug, 2021 3 commits
-
-
Oran Agra authored
-
Jonah H. Harris authored
Add SINTERCARD and ZINTERCARD commands that are similar to ZINTER and SINTER but only return the cardinality with minimum processing and memory overheads. Co-authored-by:
Oran Agra <oran@redislabs.com>
-
Ariel Shtul authored
Add new Module APS for RESP3 responses: - RM_ReplyWithMap - RM_ReplyWithSet - RM_ReplyWithAttribute - RM_ReplySetMapLength - RM_ReplySetSetLength - RM_ReplySetAttributeLength - RM_ReplyWithBool Deprecate REDISMODULE_POSTPONED_ARRAY_LEN in favor of a generic REDISMODULE_POSTPONED_LEN Improve documentation Add tests Co-authored-by:
Guy Benoish <guy.benoish@redislabs.com> Co-authored-by:
Oran Agra <oran@redislabs.com>
-
- 02 Aug, 2021 4 commits
-
-
Yossi Gottlieb authored
Loading and unloading the shared object does not initialize global vars on alpine.
-
Huang Zhw authored
When redis-cli received ASK, it used string matching wrong and didn't handle it. When we access a slot which is in migrating state, it maybe return ASK. After redirect to the new node, we need send ASKING command before retry the command. In this PR after redis-cli receives ASK, we send a ASKING command before send the origin command after reconnecting. Other changes: * Make redis-cli -u and -c (unix socket and cluster mode) incompatible with one another. * When send command fails, we avoid the 2nd reconnect retry and just print the error info. Users will decide how to do next. See #9277. * Add a test faking two redis nodes in TCL to just send ASK and OK in redis protocol to test ASK behavior. Co-authored-by:
Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by:
Oran Agra <oran@redislabs.com>
-
Ning Sun authored
Add NX, XX, GT, and LT flags to EXPIRE, PEXPIRE, EXPIREAT, PEXAPIREAT. - NX - only modify the TTL if no TTL is currently set - XX - only modify the TTL if there is a TTL currently set - GT - only increase the TTL (considering non-volatile keys as infinite expire time) - LT - only decrease the TTL (considering non-volatile keys as infinite expire time) return value of the command is 0 when the operation was skipped due to one of these flags. Signed-off-by:
Ning Sun <sunng@protonmail.com>
-
menwen authored
Fixes: - When a consumer is created as a side effect, redis didn't issue a keyspace notification, nor incremented the server.dirty (affects periodic snapshots). this was a bug in XREADGROUP, XCLAIM, and XAUTOCLAIM. - When attempting to delete a non-existent consumer, don't issue a keyspace notification and don't increment server.dirty this was a bug in XGROUP DELCONSUMER Other changes: - Changed streamLookupConsumer() to always only do lookup consumer (never do implicit creation), Its last seen time is updated unless the SLC_NO_REFRESH flag is specified. - Added streamCreateConsumer() to create a new consumer. When the creation is successful, it will notify and dirty++ unless the SCC_NO_NOTIFY or SCC_NO_DIRTIFY flags is specified. - Changed streamDelConsumer() to always only do delete consumer. - Added keyspace notifications tests about stream events.
-
- 01 Aug, 2021 3 commits
-
-
Binbin authored
With an empty src key, we need to deal with two situations: 1. non-STORE: We should return emptyarray. 2. STORE: Try to delete the store key and return 0. This applies to both GEOSEARCHSTORE (new to v6.2), and also GEORADIUS STORE (which was broken since forever) This pr try to fix #9261. i.e. both STORE variants would have behaved like the non-STORE variants when the source key was missing, returning an empty array and not deleting the destination key, instead of returning 0, and deleting the destination key. Also add more tests for some commands. - GEORADIUS: wrong type src key, non existing src key, empty search, store with non existing src key, store with empty search - GEORADIUSBYMEMBER: wrong type src key, non existing src key, non existing member, store with non existing src key - GEOSEARCH: wrong type src key, non existing src key, empty search, frommember with non existing member - GEOSEARCHSTORE: wrong type key, non existing src key, fromlonlat with empty search, frommember with non existing member Co-authored-by:
Oran Agra <oran@redislabs.com>
-
Yossi Gottlieb authored
In some cases large replies on slow systems may only be partially read by the test suite, resulting with parsing errors. This fix is still timing sensitive but should greatly reduce the chances of this happening.
-
Guy Korland authored
Co-authored-by:
Yossi Gottlieb <yossigo@gmail.com>
-
- 30 Jul, 2021 1 commit
-
-
Long Dai authored
Signed-off-by:
Long Dai <long0dai@foxmail.com>
-
- 29 Jul, 2021 3 commits
-
-
Yossi Gottlieb authored
-
Wen Hui authored
The issue is that when a sentinel with the same address and IP is turned on with a different runid, its port is set to 0 but it is still present in the dictionary master->sentinels which contain all the sentinels for a master. This causes a problem when we do INFO SENTINEL because it takes the size of the dictionary of sentinels. This might also cause a problem for failover if enough sentinels have their port set to 0 since the number of voters in failover is also determined by the size of the dictionary of sentinels. This commits removes the sentinels with the port set to zero from the dictionary of sentinels. Fixes #8786
-
sundb authored
this means the assertion that checks that when deep sanitization is enabled, there are no crashes, was missing.
-
- 25 Jul, 2021 1 commit
-
-
ZhaolongLi authored
Co-authored-by:
lizhaolong.lzl <lizhaolong.lzl@B-54MPMD6R-0221.local>
-
- 21 Jul, 2021 1 commit
-
-
Huang Zhw authored
On 32 bit platform, the bit position of GETBIT/SETBIT/BITFIELD/BITCOUNT,BITPOS may overflow (see CVE-2021-32761) (#9191) GETBIT, SETBIT may access wrong address because of wrap. BITCOUNT and BITPOS may return wrapped results. BITFIELD may access the wrong address but also allocate insufficient memory and segfault (see CVE-2021-32761). This commit uses `uint64_t` or `long long` instead of `size_t`. related https://github.com/redis/redis/pull/8096 At 32bit platform: > setbit bit 4294967295 1 (integer) 0 > config set proto-max-bulk-len 536870913 OK > append bit "\xFF" (integer) 536870913 > getbit bit 4294967296 (integer) 0 When the bit index is larger than 4294967295, size_t can't hold bit index. In the past, `proto-max-bulk-len` is limit to 536870912, so there is no problem. After this commit, bit position is stored in `uint64_t` or `long long`. So when `proto-max-bulk-len > 536870912`, 32bit platforms can still be correct. For 64bit platform, this problem still exists. The major reason is bit pos 8 times of byte pos. When proto-max-bulk-len is very larger, bit pos may overflow. But at 64bit platform, we don't have so long string. So this bug may never happen. Additionally this commit add a test cost `512MB` memory which is tag as `large-memory`. Make freebsd ci and valgrind ci ignore this test.
-
- 17 Jul, 2021 1 commit
-
-
Binbin authored
in case dest key already contains the member, the dest key isn't modified, so the command shouldn't invalidate watch.
-
- 14 Jul, 2021 1 commit
-
-
Oran Agra authored
- promote the code in DEBUG PROTOCOL to addReplyBigNum - DEBUG PROTOCOL ATTRIB skips the attribute when client is RESP2 - networking.c addReply for push and attributes generate assertion when called on a RESP2 client, anything else would produce a broken protocol that clients can't handle.
-
- 11 Jul, 2021 2 commits
-
-
perryitay authored
There are two issues fixed in this commit: 1. we want to fail the EXEC command in case there is a watched key that's logically expired but not yet deleted by active expire or lazy expire. 2. we saw that currently cache time is update in every `call()` (including nested calls), this time is being also being use for the isKeyExpired comparison, we want to update the cache time only in the first call (execCommand) Co-authored-by:
Oran Agra <oran@redislabs.com>
-
Yossi Gottlieb authored
This attempts to catch any non-standard configuration where the test may fail and produce a false positive.
-
- 07 Jul, 2021 1 commit
-
-
Mikhail Fesenko authored
Direct redis-cli repl prints to stderr, because --rdb can print to stdout. fflush stdout after responses (#9136) 1. redis-cli can output --rdb data to stdout but redis-cli also write some messages to stdout which will mess up the rdb. 2. Make redis-cli flush stdout when printing a reply This was needed in order to fix a hung in redis-cli test that uses --replica. Note that printf does flush when there's a newline, but fwrite does not. 3. fix the redis-cli --replica test which used to pass previously because it didn't really care what it read, and because redis-cli used printf to print these other things to stdout. 4. improve redis-cli --replica test to run with both diskless and disk-based. Co-authored-by:
Oran Agra <oran@redislabs.com> Co-authored-by:
Viktor Söderqvist <viktor@zuiderkwast.se>
-
- 05 Jul, 2021 1 commit
-
-
Binbin authored
due to a copy-paste bug, it used to reply with null response rather than empty array. this commit includes new tests that are looking at the RESP response directly in order to be able to tell the difference between them. Co-authored-by:
Oran Agra <oran@redislabs.com>
-
- 04 Jul, 2021 2 commits
-
-
Oran Agra authored
This makes it possible to distinguish between null response and an empty array (currently the tests infra translates both to an empty string/list)
-
Oran Agra authored
fixes test issue introduced in #9167 1. invalid reads due to accessing non-retained string (passed as unblock context). 2. leaking module blocked client context, see #6922 for info.
-
- 01 Jul, 2021 1 commit
-
-
Yossi Gottlieb authored
Modules that use background threads with thread safe contexts are likely to use RM_BlockClient() without a timeout function, because they do not set up a timeout. Before this commit, `CLIENT UNBLOCK` would result with a crash as the `NULL` timeout callback is called. Beyond just crashing, this is also logically wrong as it may throw the module into an unexpected client state. This commits makes `CLIENT UNBLOCK` on such clients behave the same as any other client that is not in a blocked state and therefore cannot be unblocked.
-
- 30 Jun, 2021 2 commits
-
-
Binbin authored
* Add keyname tags to avoid CROSSSLOT errors in external server CI * Use new wait_for_blocked_clients_count in pause.tcl
-
Binbin authored
*** [err]: PSYNC2: total sum of full synchronizations is exactly 4 intests/integration/psync2.tcl Expected 5 == 4 (context: type eval line 8 cmd {assert {$sum == 4}} proc::test) Sometime the test got an unexpected full sync since a replica switch to master, before the new master change propagated the new replid to all replicas, a replica attempted to sync with it using a wrong replid and triggered a full resync. Consider this scenario: 1 slaveof 4 full resync 0 slaveof 4 full resync 2 slaveof 0 full resync 3 slaveof 1 full resync 1 slaveof no one, replid changed 3 reconnect 1, did a partial resyn and got the new replid Before 2 inherits the new replid. 3 slaveof 2 3 try to do a partial resyn with 2. But their replication ids are inconsistent, so a full resync happens. :) A special thank you for oran and helping me in this test case. Co-authored-by:
Oran Agra <oran@redislabs.com>
-
- 29 Jun, 2021 3 commits
-
-
Yossi Gottlieb authored
-
Leibale Eidelman authored
mistakenly it used to return an empty array rather than 0. Co-authored-by:
Oran Agra <oran@redislabs.com>
-
Binbin authored
Return a bad score when used with negative count (or count of 1), and non-ziplist encoded zset. Also add test to validate the return value and cover the issue.
-
- 24 Jun, 2021 2 commits
-
-
Yossi Gottlieb authored
In the past, the first bind address that was explicitly specified was also used to bind outgoing connections. This could result with some problems. For example: on some systems using `bind 127.0.0.1` would result with outgoing connections also binding to `127.0.0.1` and failing to connect to remote addresses. With the recent change to the way `bind` is handled, this presented other issues: * The default first bind address is '*' which is not a valid address. * We make no distinction between user-supplied config that is identical to the default, and the default config. This commit addresses both these issues by introducing an explicit configuration parameter to control the bind address on outgoing connections.
-
Oran Agra authored
-
- 22 Jun, 2021 7 commits
-
-
Oran Agra authored
- Introduce a new sdssubstr api as a building block for sdsrange. The API of sdsrange is many times hard to work with and also has corner case that cause bugs. sdsrange is easy to work with and also simplifies the implementation of sdsrange. - Revert the fix to RM_StringTruncate and just use sdssubstr instead of sdsrange. - Solve valgrind warnings from the new tests introduced by the previous PR.
-
Yossi Gottlieb authored
-
Yossi Gottlieb authored
-
Yossi Gottlieb authored
* Specifying an empty `bind ""` configuration prevents Redis from listening on any TCP port. Before this commit, such configuration was not accepted. * Using `CONFIG GET bind` will always return an explicit configuration value. Before this commit, if a bind address was not specified the returned value was empty (which was an anomaly). Another behavior change is that modifying the `bind` configuration to a non-default value will NO LONGER DISABLE protected-mode implicitly.
-
Evan authored
Previously, passing 0 for newlen would not truncate the string at all. This adds handling of this case, freeing the old string and creating a new empty string. Other changes: - Move `src/modules/testmodule.c` to `tests/modules/basics.c` - Introduce that basic test into the test suite - Add tests to cover StringTruncate - Add `test-modules` build target for the main makefile - Extend `distclean` build target to clean modules too
-
Oran Agra authored
# replication-3.tcl had a test timeout failure with valgrind on daily CI: ``` *** [err]: SLAVE can reload "lua" AUX RDB fields of duplicated scripts in tests/integration/replication-3.tcl Replication not started. ``` replication took more than 70 seconds. https://github.com/redis/redis/runs/2854037905?check_suite_focus=true on my machine it takes only about 30, but i can see how 50 seconds isn't enough. # replication.tcl loading was over too quickly in freebsd daily CI: ``` *** [err]: slave fails full sync and diskless load swapdb recovers it in tests/integration/replication.tcl Expected '0' to be equal to '1' (context: type eval line 44 cmd {assert_equal [s -1 loading] 1} proc ::start_server) ``` # rdb.tcl loading was over too quickly. increase the time loading takes, and decrease the amount of work we try to achieve in that time.
-
Oran Agra authored
The `Tracking gets notification of expired keys` test in tracking.tcl used to hung in valgrind CI quite a lot. It turns out the reason is that with valgrind and a busy machine, the server cron active expire cycle could easily run in the same event loop as the command that created `mykey`, so that when they key got expired, there were two change events to broadcast, one that set the key and one that expired it, but since we used raxTryInsert, the client that was associated with the "last" change was the one that created the key, so the NOLOOP filtered that event. This commit adds a test that reproduces the problem by using lazy expire in a multi-exec which makes sure the key expires in the same event loop as the one that added it.
-