1. 11 Mar, 2023 1 commit
    • guybe7's avatar
      Add reply_schema to command json files (internal for now) (#10273) · 4ba47d2d
      guybe7 authored
      Work in progress towards implementing a reply schema as part of COMMAND DOCS, see #9845
      Since ironing the details of the reply schema of each and every command can take a long time, we
      would like to merge this PR when the infrastructure is ready, and let this mature in the unstable branch.
      Meanwhile the changes of this PR are internal, they are part of the repo, but do not affect the produced build.
      
      ### Background
      In #9656 we add a lot of information about Redis commands, but we are missing information about the replies
      
      ### Motivation
      1. Documentation. This is the primary goal.
      2. It should be possible, based on the output of COMMAND, to be able to generate client code in typed
        languages. In order to do that, we need Redis to tell us, in detail, what each reply looks like.
      3. We would like to build a fuzzer that verifies the reply structure (for now we use the existing
        testsuite, see the "Testing" section)
      
      ### Schema
      The idea is to supply some sort of schema for the various replies of each command.
      The schema will describe the conceptual structure of the reply (for generated clients), as defined in RESP3.
      Note that the reply structure itself may change, depending on the arguments (e.g. `XINFO STREAM`, with
      and without the `FULL` modifier)
      We decided to use the standard json-schema (see https://json-schema.org/) as the reply-schema.
      
      Example for `BZPOPMIN`:
      ```
      "reply_schema": {
          "oneOf": [
              {
                  "description": "Timeout reached and no elements were popped.",
                  "type": "null"
              },
              {
                  "description": "The keyname, popped member, and its score.",
                  "type": "array",
                  "minItems": 3,
                  "maxItems": 3,
                  "items": [
                      {
                          "description": "Keyname",
                          "type": "string"
                      },
                      {
                          "description": "Member",
                          "type": "string"
                      },
                      {
                          "description": "Score",
                          "type": "number"
                      }
                  ]
              }
          ]
      }
      ```
      
      #### Notes
      1.  It is ok that some commands' reply structure depends on the arguments and it's the caller's responsibility
        to know which is the relevant one. this comes after looking at other request-reply systems like OpenAPI,
        where the reply schema can also be oneOf and the caller is responsible to know which schema is the relevant one.
      2. The reply schemas will describe RESP3 replies only. even though RESP3 is structured, we want to use reply
        schema for documentation (and possibly to create a fuzzer that validates the replies)
      3. For documentation, the description field will include an explanation of the scenario in which the reply is sent,
        including any relation to arguments. for example, for `ZRANGE`'s two schemas we will need to state that one
        is with `WITHSCORES` and the other is without.
      4. For documentation, there will be another optional field "notes" in which we will add a short description of
        the representation in RESP2, in case it's not trivial (RESP3's `ZRANGE`'s nested array vs. RESP2's flat
        array, for example)
      
      Given the above:
      1. We can generate the "return" section of all commands in [redis-doc](https://redis.io/commands/)
        (given that "description" and "notes" are comprehensive enough)
      2. We can generate a client in a strongly typed language (but the return type could be a conceptual
        `union` and the caller needs to know which schema is relevant). see the section below for RESP2 support.
      3. We can create a fuzzer for RESP3.
      
      ### Limitations (because we are using the standard json-schema)
      The problem is that Redis' replies are more diverse than what the json format allows. This means that,
      when we convert the reply to a json (in order to validate the schema against it), we lose information (see
      the "Testing" section below).
      The other option would have been to extend the standard json-schema (and json format) to include stuff
      like sets, bulk-strings, error-string, etc. but that would mean also extending the schema-validator - and that
      seemed like too much work, so we decided to compromise.
      
      Examples:
      1. We cannot tell the difference between an "array" and a "set"
      2. We cannot tell the difference between simple-string and bulk-string
      3. we cannot verify true uniqueness of items in commands like ZRANGE: json-schema doesn't cover the
        case of two identical members with different scores (e.g. `[["m1",6],["m1",7]]`) because `uniqueItems`
        compares (member,score) tuples and not just the member name. 
      
      ### Testing
      This commit includes some changes inside Redis in order to verify the schemas (existing and future ones)
      are indeed correct (i.e. describe the actual response of Redis).
      To do that, we added a debugging feature to Redis that causes it to produce a log of all the commands
      it executed and their replies.
      For that, Redis needs to be compiled with `-DLOG_REQ_RES` and run with
      `--reg-res-logfile <file> --client-default-resp 3` (the testsuite already does that if you run it with
      `--log-req-res --force-resp3`)
      You should run the testsuite with the above args (and `--dont-clean`) in order to make Redis generate
      `.reqres` files (same dir as the `stdout` files) which contain request-response pairs.
      These files are later on processed by `./utils/req-res-log-validator.py` which does:
      1. Goes over req-res files, generated by redis-servers, spawned by the testsuite (see logreqres.c)
      2. For each request-response pair, it validates the response against the request's reply_schema
        (obtained from the extended COMMAND DOCS)
      5. In order to get good coverage of the Redis commands, and all their different replies, we chose to use
        the existing redis test suite, rather than attempt to write a fuzzer.
      
      #### Notes about RESP2
      1. We will not be able to use the testing tool to verify RESP2 replies (we are ok with that, it's time to
        accept RESP3 as the future RESP)
      2. Since the majority of the test suite is using RESP2, and we want the server to reply with RESP3
        so that we can validate it, we will need to know how to convert the actual reply to the one expected.
         - number and boolean are always strings in RESP2 so the conversion is easy
         - objects (maps) are always a flat array in RESP2
         - others (nested array in RESP3's `ZRANGE` and others) will need some special per-command
           handling (so the client will not be totally auto-generated)
      
      Example for ZRANGE:
      ```
      "reply_schema": {
          "anyOf": [
              {
                  "description": "A list of member elements",
                  "type": "array",
                  "uniqueItems": true,
                  "items": {
                      "type": "string"
                  }
              },
              {
                  "description": "Members and their scores. Returned in case `WITHSCORES` was used.",
                  "notes": "In RESP2 this is returned as a flat array",
                  "type": "array",
                  "uniqueItems": true,
                  "items": {
                      "type": "array",
                      "minItems": 2,
                      "maxItems": 2,
                      "items": [
                          {
                              "description": "Member",
                              "type": "string"
                          },
                          {
                              "description": "Score",
                              "type": "number"
                          }
                      ]
                  }
              }
          ]
      }
      ```
      
      ### Other changes
      1. Some tests that behave differently depending on the RESP are now being tested for both RESP,
        regardless of the special log-req-res mode ("Pub/Sub PING" for example)
      2. Update the history field of CLIENT LIST
      3. Added basic tests for commands that were not covered at all by the testsuite
      
      ### TODO
      
      - [x] (maybe a different PR) add a "condition" field to anyOf/oneOf schemas that refers to args. e.g.
        when `SET` return NULL, the condition is `arguments.get||arguments.condition`, for `OK` the condition
        is `!arguments.get`, and for `string` the condition is `arguments.get` - https://github.com/redis/redis/issues/11896
      - [x] (maybe a different PR) also run `runtest-cluster` in the req-res logging mode
      - [x] add the new tests to GH actions (i.e. compile with `-DLOG_REQ_RES`, run the tests, and run the validator)
      - [x] (maybe a different PR) figure out a way to warn about (sub)schemas that are uncovered by the output
        of the tests - https://github.com/redis/redis/issues/11897
      - [x] (probably a separate PR) add all missing schemas
      - [x] check why "SDOWN is triggered by misconfigured instance replying with errors" fails with --log-req-res
      - [x] move the response transformers to their own file (run both regular, cluster, and sentinel tests - need to
        fight with the tcl including mechanism a bit)
      - [x] issue: module API - https://github.com/redis/redis/issues/11898
      - [x] (probably a separate PR): improve schemas: add `required` to `object`s - https://github.com/redis/redis/issues/11899
      
      Co-authored-by: default avatarOzan Tezcan <ozantezcan@gmail.com>
      Co-authored-by: default avatarHanna Fadida <hanna.fadida@redislabs.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarShaya Potter <shaya@redislabs.com>
      4ba47d2d
  2. 08 Mar, 2023 1 commit
    • ranshid's avatar
      Fix an issue when module decides to unblock a client which is blocked on keys (#11832) · 4988b928
      ranshid authored
      Currently (starting at #11012) When a module is blocked on keys it sets the
      CLIENT_PENDING_COMMAND flag.
      However in case the module decides to unblock the client not via the regular flow
      (eg timeout, key signal or CLIENT UNBLOCK command) it will attempt to reprocess the
      module command and potentially blocked again.
      
      This fix remove the CLIENT_PENDING_COMMAND flag in case blockedForKeys is
      issued from module context.
      4988b928
  3. 07 Mar, 2023 1 commit
    • sundb's avatar
      Skip test for sdsRemoveFreeSpace when mem_allocator is not jemalloc (#11878) · 3fba3ccd
      sundb authored
      Test `trim on SET with big value` (introduced from #11817) fails under mac m1 with libc mem_allocator.
      The reason is that malloc(33000) will allocate 65536 bytes(>42000).
      This test still passes under ubuntu with libc mem_allocator.
      
      ```
      *** [err]: trim on SET with big value in tests/unit/type/string.tcl
      Expected [r memory usage key] < 42000 (context: type source line 471 file /Users/iospack/data/redis_fork/tests/unit/type/string.tcl cmd {assert {[r memory usage key] < 42000}} proc ::test)
      ```
      
      simple test under mac m1 with libc mem_allocator:
      ```c
      void *p = zmalloc(33000);
      printf("malloc size: %zu\n", zmalloc_size(p));
      
      # output
      malloc size: 65536
      ```
      3fba3ccd
  4. 28 Feb, 2023 1 commit
    • uriyage's avatar
      Try to trim strings only when applicable (#11817) · 9d336ac3
      uriyage authored
      
      
      As `sdsRemoveFreeSpace` have an impact on performance even if it is a no-op (see details at #11508). 
      Only call the function when there is a possibility that the string contains free space.
      * For strings coming from the network, it's only if they're bigger than PROTO_MBULK_BIG_ARG
      * For strings coming from scripts, it's only if they're smaller than LUA_CMD_OBJCACHE_MAX_LEN
      * For strings coming from modules, it could be anything.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarsundb <sundbcn@gmail.com>
      9d336ac3
  5. 16 Feb, 2023 1 commit
    • Oran Agra's avatar
      Cleanup around script_caller, fix tracking of scripts and ACL logging for RM_Call (#11770) · 233abbbe
      Oran Agra authored
      * Make it clear that current_client is the root client that was called by
        external connection
      * add executing_client which is the client that runs the current command
        (can be a module or a script)
      * Remove script_caller that was used for commands that have CLIENT_SCRIPT
        to get the client that called the script. in most cases, that's the current_client,
        and in others (when being called from a module), it could be an intermediate
        client when we actually want the original one used by the external connection.
      
      bugfixes:
      * RM_Call with C flag should log ACL errors with the requested user rather than
        the one used by the original client, this also solves a crash when RM_Call is used
        with C flag from a detached thread safe context.
      * addACLLogEntry would have logged info about the script_caller, but in case the
        script was issued by a module command we actually want the current_client. the
        exception is when RM_Call is called from a timer event, in which case we don't
        have a current_client.
      
      behavior changes:
      * client side tracking for scripts now tracks the keys that are read by the script
        instead of the keys that are declared by the caller for EVAL
      
      other changes:
      * Log both current_client and executing_client in the crash log.
      * remove prepareLuaClient and resetLuaClient, being dead code that was forgotten.
      * remove scriptTimeSnapshot and snapshot_time and instead add cmd_time_snapshot
        that serves all commands and is reset only when execution nesting starts.
      * remove code to propagate CLIENT_FORCE_REPL from the executed command
        to the script caller since scripts aren't propagated anyway these days and anyway
        this flag wouldn't have had an effect since CLIENT_PREVENT_PROP is added by scriptResetRun.
      * fix a module GIL violation issue in afterSleep that was introduced in #10300 (unreleased)
      233abbbe
  6. 14 Feb, 2023 1 commit
    • guybe7's avatar
      Minor changes around the blockonkeys test module (#11803) · 9483ab0b
      guybe7 authored
      All of the POP commands must not decr length below 0.
      So, get_fsl will delete the key if the length is 0 (unless
      the caller wished to create if doesn't exist)
      
      Other:
      1. Use REDISMODULE_WRITE where needed (POP commands)
      2. Use wait_for_blokced_clients in test
      
      Unrelated:
      Use quotes instead of curly braces in zset.tcl, for variable expansion
      9483ab0b
  7. 09 Feb, 2023 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Match REDISMODULE_OPEN_KEY_* flags to LOOKUP_* flags (#11772) · 5c3938d5
      Meir Shpilraien (Spielrein) authored
      The PR adds support for the following flags on RedisModule_OpenKey:
      
      * REDISMODULE_OPEN_KEY_NONOTIFY - Don't trigger keyspace event on key misses.
      * REDISMODULE_OPEN_KEY_NOSTATS - Don't update keyspace hits/misses counters.
      * REDISMODULE_OPEN_KEY_NOEXPIRE - Avoid deleting lazy expired keys.
      * REDISMODULE_OPEN_KEY_NOEFFECTS - Avoid any effects from fetching the key
      
      In addition, added `RM_GetOpenKeyModesAll`, which returns the mask of all
      supported OpenKey modes. This allows the module to check, in runtime, which
      OpenKey modes are supported by the current Redis instance.
      5c3938d5
  8. 04 Jan, 2023 1 commit
    • Oran Agra's avatar
      Fix potential issue with Lua argv caching, module command filter and libc realloc (#11652) · c8052122
      Oran Agra authored
      TLDR: solve a problem introduced in Redis 7.0.6 (#11541) with
      RM_CommandFilterArgInsert being called from scripts, which can
      lead to memory corruption.
      
      Libc realloc can return the same pointer even if the size was changed. The code in
      freeLuaRedisArgv had an assumption that if the pointer didn't change, then the
      allocation didn't change, and the cache can still be reused.
      However, if rewriteClientCommandArgument or RM_CommandFilterArgInsert were
      used, it could be that we realloced the argv array, and the pointer didn't change, then
      a consecutive command being executed from Lua can use that argv cache reaching
      beyond its size.
      This was actually only possible with modules, since the decision to realloc was based
      on argc, rather than argv_len.
      c8052122
  9. 01 Jan, 2023 1 commit
    • ranshid's avatar
      reprocess command when client is unblocked on keys (#11012) · 383d902c
      ranshid authored
      *TL;DR*
      ---------------------------------------
      Following the discussion over the issue [#7551](https://github.com/redis/redis/issues/7551
      
      )
      We decided to refactor the client blocking code to eliminate some of the code duplications
      and to rebuild the infrastructure better for future key blocking cases.
      
      
      *In this PR*
      ---------------------------------------
      1. reprocess the command once a client becomes unblocked on key (instead of running
         custom code for the unblocked path that's different than the one that would have run if
         blocking wasn't needed)
      2. eliminate some (now) irrelevant code for handling unblocking lists/zsets/streams etc...
      3. modify some tests to intercept the error in cases of error on reprocess after unblock (see
         details in the notes section below)
      4. replace '$' on the client argv with current stream id. Since once we reprocess the stream
         XREAD we need to read from the last msg and not wait for new msg  in order to prevent
         endless block loop. 
      5. Added statistics to the info "Clients" section to report the:
         * `total_blocking_keys` - number of blocking keys
         * `total_blocking_keys_on_nokey` - number of blocking keys which have at least 1 client
            which would like
         to be unblocked on when the key is deleted.
      6. Avoid expiring unblocked key during unblock. Previously we used to lookup the unblocked key
         which might have been expired during the lookup. Now we lookup the key using NOTOUCH and
         NOEXPIRE to avoid deleting it at this point, so propagating commands in blocked.c is no longer needed.
      7. deprecated command flags. We decided to remove the CMD_CALL_STATS and CMD_CALL_SLOWLOG
         and make an explicit verification in the call() function in order to decide if stats update should take place.
         This should simplify the logic and also mitigate existing issues: for example module calls which are
         triggered as part of AOF loading might still report stats even though they are called during AOF loading.
      
      *Behavior changes*
      ---------------------------------------------------
      
      1. As this implementation prevents writing dedicated code handling unblocked streams/lists/zsets,
      since we now re-process the command once the client is unblocked some errors will be reported differently.
      The old implementation used to issue
      ``UNBLOCKED the stream key no longer exists``
      in the following cases:
         - The stream key has been deleted (ie. calling DEL)
         - The stream and group existed but the key type was changed by overriding it (ie. with set command)
         - The key not longer exists after we swapdb with a db which does not contains this key
         - After swapdb when the new db has this key but with different type.
         
      In the new implementation the reported errors will be the same as if the command was processed after effect:
      **NOGROUP** - in case key no longer exists, or **WRONGTYPE** in case the key was overridden with a different type.
      
      2. Reprocessing the command means that some checks will be reevaluated once the
      client is unblocked.
      For example, ACL rules might change since the command originally was executed and
      will fail once the client is unblocked.
      Another example is OOM condition checks which might enable the command to run and
      block but fail the command reprocess once the client is unblocked.
      
      3. One of the changes in this PR is that no command stats are being updated once the
      command is blocked (all stats will be updated once the client is unblocked). This implies
      that when we have many clients blocked, users will no longer be able to get that information
      from the command stats. However the information can still be gathered from the client list.
      
      **Client blocking**
      ---------------------------------------------------
      
      the blocking on key will still be triggered the same way as it is done today.
      in order to block the current client on list of keys, the call to
      blockForKeys will still need to be made which will perform the same as it is today:
      
      *  add the client to the list of blocked clients on each key
      *  keep the key with a matching list node (position in the global blocking clients list for that key)
         in the client private blocking key dict.
      *  flag the client with CLIENT_BLOCKED
      *  update blocking statistics
      *  register the client on the timeout table
      
      **Key Unblock**
      ---------------------------------------------------
      
      Unblocking a specific key will be triggered (same as today) by calling signalKeyAsReady.
      the implementation in that part will stay the same as today - adding the key to the global readyList.
      The reason to maintain the readyList (as apposed to iterating over all clients blocked on the specific key)
      is in order to keep the signal operation as short as possible, since it is called during the command processing.
      The main change is that instead of going through a dedicated code path that operates the blocked command
      we will just call processPendingCommandsAndResetClient.
      
      **ClientUnblock (keys)**
      ---------------------------------------------------
      
      1. Unblocking clients on keys will be triggered after command is
         processed and during the beforeSleep
      8. the general schema is:
      9. For each key *k* in the readyList:
      ```            
      For each client *c* which is blocked on *k*:
                  in case either:
      	          1. *k* exists AND the *k* type matches the current client blocking type
      	  	      OR
      	          2. *k* exists and *c* is blocked on module command
      	    	      OR
      	          3. *k* does not exists and *c* was blocked with the flag
      	             unblock_on_deleted_key
                       do:
                                        1. remove the client from the list of clients blocked on this key
                                        2. remove the blocking list node from the client blocking key dict
                                        3. remove the client from the timeout list
                                        10. queue the client on the unblocked_clients list
                                        11. *NEW*: call processCommandAndResetClient(c);
      ```
      *NOTE:* for module blocked clients we will still call the moduleUnblockClientByHandle
                    which will queue the client for processing in moduleUnblockedClients list.
      
      **Process Unblocked clients**
      ---------------------------------------------------
      
      The process of all unblocked clients is done in the beforeSleep and no change is planned
      in that part.
      
      The general schema will be:
      For each client *c* in server.unblocked_clients:
      
              * remove client from the server.unblocked_clients
              * set back the client readHandler
              * continue processing the pending command and input buffer.
      
      *Some notes regarding the new implementation*
      ---------------------------------------------------
      
      1. Although it was proposed, it is currently difficult to remove the
         read handler from the client while it is blocked.
         The reason is that a blocked client should be unblocked when it is
         disconnected, or we might consume data into void.
      
      2. While this PR mainly keep the current blocking logic as-is, there
         might be some future additions to the infrastructure that we would
         like to have:
         - allow non-preemptive blocking of client - sometimes we can think
           that a new kind of blocking can be expected to not be preempt. for
           example lets imagine we hold some keys on disk and when a command
           needs to process them it will block until the keys are uploaded.
           in this case we will want the client to not disconnect or be
           unblocked until the process is completed (remove the client read
           handler, prevent client timeout, disable unblock via debug command etc...).
         - allow generic blocking based on command declared keys - we might
           want to add a hook before command processing to check if any of the
           declared keys require the command to block. this way it would be
           easier to add new kinds of key-based blocking mechanisms.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Signed-off-by: default avatarRan Shidlansik <ranshid@amazon.com>
      383d902c
  10. 20 Dec, 2022 1 commit
    • guybe7's avatar
      Cleanup: Get rid of server.core_propagates (#11572) · 9c7c6924
      guybe7 authored
      1. Get rid of server.core_propagates - we can just rely on module/call nesting levels
      2. Rename in_nested_call  to execution_nesting and update the comment
      3. Remove module_ctx_nesting (redundant, we can use execution_nesting)
      4. Modify postExecutionUnitOperations according to the comment (The main purpose of this PR)
      5. trackingHandlePendingKeyInvalidations: Check the nesting level inside this function
      9c7c6924
  11. 09 Dec, 2022 1 commit
    • Binbin's avatar
      Fix zuiFind crash / RM_ScanKey hang on SET object listpack encoding (#11581) · 20854cb6
      Binbin authored
      
      
      In #11290, we added listpack encoding for SET object.
      But forgot to support it in zuiFind, causes ZINTER, ZINTERSTORE,
      ZINTERCARD, ZIDFF, ZDIFFSTORE to crash.
      And forgot to support it in RM_ScanKey, causes it hang.
      
      This PR add support SET listpack in zuiFind, and in RM_ScanKey.
      And add tests for related commands to cover this case.
      
      Other changes:
      - There is no reason for zuiFind to go into the internals of the SET.
        It can simply use setTypeIsMember and don't care about encoding.
      - Remove the `#include "intset.h"` from server.h reduce the chance of
        accidental intset API use.
      - Move setTypeAddAux, setTypeRemoveAux and setTypeIsMemberAux
        interfaces to the header.
      - In scanGenericCommand, use setTypeInitIterator and setTypeNext
        to handle OBJ_SET scan.
      - In RM_ScanKey, improve hash scan mode, use lpGetValue like zset,
        they can share code and better performance.
      
      The zuiFind part fixes #11578
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarViktor Söderqvist <viktor.soderqvist@est.tech>
      20854cb6
  12. 08 Dec, 2022 1 commit
  13. 30 Nov, 2022 1 commit
    • Huang Zhw's avatar
      Add a special notification unlink available only for modules (#9406) · c8181314
      Huang Zhw authored
      
      
      Add a new module event `RedisModule_Event_Key`, this event is fired
      when a key is removed from the keyspace.
      The event includes an open key that can be used for reading the key before
      it is removed. Modules can also extract the key-name, and use RM_Open
      or RM_Call to access key from within that event, but shouldn't modify anything
      from within this event.
      
      The following sub events are available:
        - `REDISMODULE_SUBEVENT_KEY_DELETED`
        - `REDISMODULE_SUBEVENT_KEY_EXPIRED`
        - `REDISMODULE_SUBEVENT_KEY_EVICTED`
        - `REDISMODULE_SUBEVENT_KEY_OVERWRITE`
      
      The data pointer can be casted to a RedisModuleKeyInfo structure
      with the following fields:
      ```
           RedisModuleKey *key;    // Opened Key
       ```
      
      ### internals
      
      * We also add two dict functions:
        `dictTwoPhaseUnlinkFind` finds an element from the table, also get the plink of the entry.
        The entry is returned if the element is found. The user should later call `dictTwoPhaseUnlinkFree`
        with it in order to unlink and release it. Otherwise if the key is not found, NULL is returned.
        These two functions should be used in pair. `dictTwoPhaseUnlinkFind` pauses rehash and
        `dictTwoPhaseUnlinkFree` resumes rehash.
      * We change `dbOverwrite` to `dbReplaceValue` which just replaces the value of the key and
        doesn't fire any events. The "overwrite" part (which emits events) is just when called from `setKey`,
        the other places that called dbOverwrite were ones that just update the value in-place (INCR*, SPOP,
        and dbUnshareStringValue). This should not have any real impact since `moduleNotifyKeyUnlink` and
        `signalDeletedKeyAsReady` wouldn't have mattered in these cases anyway (i.e. module keys and
        stream keys didn't have direct calls to dbOverwrite)
      * since we allow doing RM_OpenKey from withing these callbacks, we temporarily disable lazy expiry.
      * We also temporarily disable lazy expiry when we are in unlink/unlink2 callback and keyspace 
        notification callback.
      * Move special definitions to the top of redismodule.h
        This is needed to resolve compilation errors with RedisModuleKeyInfoV1
        that carries a RedisModuleKey member.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      c8181314
  14. 24 Nov, 2022 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Module API to allow writes after key space notification hooks (#11199) · abc345ad
      Meir Shpilraien (Spielrein) authored
      ### Summary of API additions
      
      * `RedisModule_AddPostNotificationJob` - new API to call inside a key space
        notification (and on more locations in the future) and allow to add a post job as describe above.
      * New module option, `REDISMODULE_OPTIONS_ALLOW_NESTED_KEYSPACE_NOTIFICATIONS`,
        allows to disable Redis protection of nested key-space notifications.
      * `RedisModule_GetModuleOptionsAll` - gets the mask of all supported module options so a module
        will be able to check if a given option is supported by the current running Redis instance.
      
      ### Background
      
      The following PR is a proposal of handling write operations inside module key space notifications.
      After a lot of discussions we came to a conclusion that module should not perform any write
      operations on key space notification.
      
      Some examples of issues that such write operation can cause are describe on the following links:
      
      * Bad replication oreder - https://github.com/redis/redis/pull/10969
      * Used after free - https://github.com/redis/redis/pull/10969#issuecomment-1223771006
      * Used after free - https://github.com/redis/redis/pull/9406#issuecomment-1221684054
      
      
      
      There are probably more issues that are yet to be discovered. The underline problem with writing
      inside key space notification is that the notification runs synchronously, this means that the notification
      code will be executed in the middle on Redis logic (commands logic, eviction, expire).
      Redis **do not assume** that the data might change while running the logic and such changes
      can crash Redis or cause unexpected behaviour.
      
      The solution is to state that modules **should not** perform any write command inside key space
      notification (we can chose whether or not we want to force it). To still cover the use-case where
      module wants to perform a write operation as a reaction to key space notifications, we introduce
      a new API , `RedisModule_AddPostNotificationJob`, that allows to register a callback that will be
      called by Redis when the following conditions hold:
      
      * It is safe to perform any write operation.
      * The job will be called atomically along side the operation that triggers it (in our case, key
        space notification).
      
      Module can use this new API to safely perform any write operation and still achieve atomicity
      between the notification and the write.
      
      Although currently the API is supported on key space notifications, the API is written in a generic
      way so that in the future we will be able to use it on other places (server events for example).
      
      ### Technical Details
      
      Whenever a module uses `RedisModule_AddPostNotificationJob` the callback is added to a list
      of callbacks (called `modulePostExecUnitJobs`) that need to be invoke after the current execution
      unit ends (whether its a command, eviction, or active expire). In order to trigger those callback
      atomically with the notification effect, we call those callbacks on `postExecutionUnitOperations`
      (which was `propagatePendingCommands` before this PR). The new function fires the post jobs
      and then calls `propagatePendingCommands`.
      
      If the callback perform more operations that triggers more key space notifications. Those keys
      space notifications might register more callbacks. Those callbacks will be added to the end
      of `modulePostExecUnitJobs` list and will be invoke atomically after the current callback ends.
      This raises a concerns of entering an infinite loops, we consider infinite loops as a logical bug
      that need to be fixed in the module, an attempt to protect against infinite loops by halting the
      execution could result in violation of the feature correctness and so **Redis will make no attempt
      to protect the module from infinite loops**
      
      In addition, currently key space notifications are not nested. Some modules might want to allow
      nesting key-space notifications. To allow that and keep backward compatibility, we introduce a
      new module option called `REDISMODULE_OPTIONS_ALLOW_NESTED_KEYSPACE_NOTIFICATIONS`.
      Setting this option will disable the Redis key-space notifications nesting protection and will
      pass this responsibility to the module.
      
      ### Redis infrastructure
      
      This PR promotes the existing `propagatePendingCommands` to an "Execution Unit" concept,
      which is called after each atomic unit of execution,
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarYossi Gottlieb <yossigo@gmail.com>
      Co-authored-by: default avatarMadelyn Olson <34459052+madolson@users.noreply.github.com>
      abc345ad
  15. 16 Nov, 2022 1 commit
    • sundb's avatar
      Add listpack encoding for list (#11303) · 2168ccc6
      sundb authored
      Improve memory efficiency of list keys
      
      ## Description of the feature
      The new listpack encoding uses the old `list-max-listpack-size` config
      to perform the conversion, which we can think it of as a node inside a
      quicklist, but without 80 bytes overhead (internal fragmentation included)
      of quicklist and quicklistNode structs.
      For example, a list key with 5 items of 10 chars each, now takes 128 bytes
      instead of 208 it used to take.
      
      ## Conversion rules
      * Convert listpack to quicklist
        When the listpack length or size reaches the `list-max-listpack-size` limit,
        it will be converted to a quicklist.
      * Convert quicklist to listpack
        When a quicklist has only one node, and its length or size is reduced to half
        of the `list-max-listpack-size` limit, it will be converted to a listpack.
        This is done to avoid frequent conversions when we add or remove at the bounding size or length.
          
      ## Interface changes
      1. add list entry param to listTypeSetIteratorDirection
          When list encoding is listpack, `listTypeIterator->lpi` points to the next entry of current entry,
          so when changing the direction, we need to use the current node (listTypeEntry->p) to 
          update `listTypeIterator->lpi` to the next node in the reverse direction.
      
      ## Benchmark
      ### Listpack VS Quicklist with one node
      * LPUSH - roughly 0.3% improvement
      * LRANGE - roughly 13% improvement
      
      ### Both are quicklist
      * LRANGE - roughly 3% improvement
      * LRANGE without pipeline - roughly 3% improvement
      
      From the benchmark, as we can see from the results
      1. When list is quicklist encoding, LRANGE improves performance by <5%.
      2. When list is listpack encoding, LRANGE improves performance by ~13%,
         the main enhancement is brought by `addListListpackRangeReply()`.
      
      ## Memory usage
      1M lists(key:0~key:1000000) with 5 items of 10 chars ("hellohello") each.
      shows memory usage down by 35.49%, from 214MB to 138MB.
      
      ## Note
      1. Add conversion callback to support doing some work before conversion
          Since the quicklist iterator decompresses the current node when it is released, we can 
          no longer decompress the quicklist after we convert the list.
      2168ccc6
  16. 15 Nov, 2022 1 commit
    • Binbin's avatar
      Fix double negative nan test, ignoring sign (#11506) · a4bcdbcf
      Binbin authored
      The test introduced in #11482 fail on ARM (extra CI):
      ```
      *** [err]: RESP2: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl
      Expected '-nan' to be equal to 'nan' (context: type eval line 3 cmd
      {assert_equal "-nan" [r rw.double 0 0]} proc ::test)
      
      *** [err]: RESP3: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl
      Expected ',-nan' to be equal to ',nan' (context: type eval line 8 cmd
      {assert_equal ",-nan" [r rw.double 0 0]} proc ::test)
      ```
      
      It looks like there is no negative nan on ARM. 
      a4bcdbcf
  17. 14 Nov, 2022 2 commits
  18. 13 Nov, 2022 1 commit
  19. 27 Oct, 2022 1 commit
    • Shaya Potter's avatar
      RM_Call - only enforce OOM on scripts if 'M' flag is sent (#11425) · 38028dab
      Shaya Potter authored
      
      
      RM_Call is designed to let modules call redis commands disregarding the
      OOM state (the module is responsible to declare its command flags to redis,
      or perform the necessary checks).
      The other (new) alternative is to pass the "M" flag to RM_Call so that redis can
      OOM reject commands implicitly.
      
      However, Currently, RM_Call enforces OOM on scripts (excluding scripts that
      declared `allow-oom`) in all cases, regardless of the RM_Call "M" flag being present.
      
      This PR fixes scripts to be consistent with other commands being executed by RM_Call.
      It modifies the flow in effect treats scripts as if they if they have the ALLOW_OOM script
      flag, if the "M" flag is not passed (i.e. no OOM checking is being performed by RM_Call,
      so no OOM checking should be done on script).
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      38028dab
  20. 22 Oct, 2022 1 commit
    • sundb's avatar
      Fix crash due to to reuse iterator entry after list deletion in module (#11383) · 6dd21355
      sundb authored
      
      
      In the module, we will reuse the list iterator entry for RM_ListDelete, but `listTypeDelete` will only update
      `quicklistEntry->zi` but not `quicklistEntry->node`, which will result in `quicklistEntry->node` pointing to
      a freed memory address if the quicklist node is deleted. 
      
      This PR sync `key->u.list.index` and `key->u.list.entry` to list iterator after `RM_ListDelete`.
      
      This PR also optimizes the release code of the original list iterator.
      Co-authored-by: default avatarViktor Söderqvist <viktor@zuiderkwast.se>
      6dd21355
  21. 18 Oct, 2022 2 commits
    • guybe7's avatar
      Blocked module clients should be aware when a key is deleted (#11310) · b57fd010
      guybe7 authored
      The use case is a module that wants to implement a blocking command on a key that
      necessarily exists and wants to unblock the client in case the key is deleted (much like
      what we implemented for XREADGROUP in #10306)
      
      New module API:
      * RedisModule_BlockClientOnKeysWithFlags
      
      Flags:
      * REDISMODULE_BLOCK_UNBLOCK_NONE
      * REDISMODULE_BLOCK_UNBLOCK_DELETED
      
      ### Detailed description of code changes
      
      blocked.c:
      1. Both module and stream functions are called whether the key exists or not, regardless of
        its type. We do that in order to allow modules/stream to unblock the client in case the key
        is no longer present or has changed type (the behavior for streams didn't change, just code
        that moved into serveClientsBlockedOnStreamKey)
      2. Make sure afterCommand is called in serveClientsBlockedOnKeyByModule, in order to propagate
        actions from moduleTryServeClientBlockedOnKey.
      3. handleClientsBlockedOnKeys: call propagatePendingCommands directly after lookupKeyReadWithFlags
        to prevent a possible lazy-expire DEL from being mixed with any command propagated by the
        preceding functions.
      4. blockForKeys: Caller can specifiy that it wants to be awakened if key is deleted.
         Minor optimizations (use dictAddRaw).
      5. signalKeyAsReady became signalKeyAsReadyLogic which can take a boolean in case the key is deleted.
        It will only signal if there's at least one client that awaits key deletion (to save calls to
        handleClientsBlockedOnKeys).
        Minor optimizations (use dictAddRaw)
      
      db.c:
      1. scanDatabaseForDeletedStreams is now scanDatabaseForDeletedKeys and will signalKeyAsReady
        for any key that was removed from the database or changed type. It is the responsibility of the code
        in blocked.c to ignore or act on deleted/type-changed keys.
      2. Use the new signalDeletedKeyAsReady where needed
      
      blockedonkey.c + tcl:
      1. Added test of new capabilities (FSL.BPOPGT now requires the key to exist in order to work)
      b57fd010
    • Meir Shpilraien (Spielrein)'s avatar
      Avoid saving module aux on RDB if no aux data was saved by the module. (#11374) · b43f2548
      Meir Shpilraien (Spielrein) authored
      ### Background
      
      The issue is that when saving an RDB with module AUX data, the module AUX metadata
      (moduleid, when, ...) is saved to the RDB even though the module did not saved any actual data.
      This prevent loading the RDB in the absence of the module (although there is no actual data in
      the RDB that requires the module to be loaded).
      
      ### Solution
      
      The solution suggested in this PR is that module AUX will be saved on the RDB only if the module
      actually saved something during `aux_save` function.
      
      To support backward compatibility, we introduce `aux_save2` callback that acts the same as
      `aux_save` with the tiny change of avoid saving the aux field if no data was actually saved by
      the module. Modules can use the new API to make sure that if they have no data to save,
      then it will be possible to load the created RDB even without the module.
      
      ### Concerns
      
      A module may register for the aux load and save hooks just in order to be notified when
      saving or loading starts or completed (there are better ways to do that, but it still possible
      that someone used it).
      
      However, if a module didn't save a single field in the save callback, it means it's not allowed
      to read in the read callback, since it has no way to distinguish between empty and non-empty
      payloads. furthermore, it means that if the module did that, it must never change it, since it'll
      break compatibility with it's old RDB files, so this is really not a valid use case.
      
      Since some modules (ones who currently save one field indicating an empty payload), need
      to know if saving an empty payload is valid, and if Redis is gonna ignore an empty payload
      or store it, we opted to add a new API (rather than change behavior of an existing API and
      expect modules to check the redis version)
      
      ### Technical Details
      
      To avoid saving AUX data on RDB, we change the code to first save the AUX metadata
      (moduleid, when, ...) into a temporary buffer. The buffer is then flushed to the rio at the first
      time the module makes a write operation inside the `aux_save` function. If the module saves
      nothing (and `aux_save2` was used), the entire temporary buffer is simply dropped and no
      data about this AUX field is saved to the RDB. This make it possible to load the RDB even in
      the absence of the module.
      
      Test was added to verify the fix.
      b43f2548
  22. 16 Oct, 2022 2 commits
    • Shaya Potter's avatar
      Unify ACL failure error messaging. (#11160) · 3193f086
      Shaya Potter authored
      Motivation: for applications that use RM ACL verification functions, they would
      want to return errors back to the user, in ways that are consistent with Redis.
      While investigating how we should return ACL errors to the user, we realized that
      Redis isn't consistent, and currently returns ACL error strings in 3 primary ways.
      
      [For the actual implications of this change, see the "Impact" section at the bottom]
      
      1. how it returns an error when calling a command normally
         ACL_DENIED_CMD -> "this user has no permissions to run the '%s' command"
         ACL_DENIED_KEY -> "this user has no permissions to access one of the keys used as arguments"
         ACL_DENIED_CHANNEL -> "this user has no permissions to access one of the channels used as arguments"
      
      2. how it returns an error when calling via 'acl dryrun' command
         ACL_DENIED_CMD ->  "This user has no permissions to run the '%s' command"
         ACL_DENIED_KEY -> "This user has no permissions to access the '%s' key"
         ACL_DENIED_CHANNEL -> "This user has no permissions to access the '%s' channel"
      
      3. how it returns an error via RM_Call (and scripting is similar).
         ACL_DENIED_CMD -> "can't run this command or subcommand";
         ACL_DENIED_KEY -> "can't access at least one of the keys mentioned in the command arguments";
         ACL_DENIED_CHANNEL -> "can't publish to the channel mentioned in the command";
         
         In addition, if one wants to use RM_Call's "dry run" capability instead of the RM ACL
         functions directly, one also sees a different problem than it returns ACL errors with a -ERR,
         not a -PERM, so it can't be returned directly to the caller.
      
      This PR modifies the code to generate a base message in a common manner with the ability
      to set verbose flag for acl dry run errors, and keep it unset for normal/rm_call/script cases
      
      ```c
      sds getAclErrorMessage(int acl_res, user *user, struct redisCommand *cmd, sds errored_val, int verbose) {
          switch (acl_res) {
          case ACL_DENIED_CMD:
              return sdscatfmt(sdsempty(), "User %S has no permissions to run "
                                           "the '%S' command", user->name, cmd->fullname);
          case ACL_DENIED_KEY:
              if (verbose) {
                  return sdscatfmt(sdsempty(), "User %S has no permissions to access "
                                               "the '%S' key", user->name, errored_val);
              } else {
                  return sdsnew("No permissions to access a key");
              }
          case ACL_DENIED_CHANNEL:
              if (verbose) {
                  return sdscatfmt(sdsempty(), "User %S has no permissions to access "
                                               "the '%S' channel", user->name, errored_val);
              } else {
                  return sdsnew("No permissions to access a channel");
              }
          }
      ```
      
      The caller can append/prepend the message (adding NOPERM for normal/RM_Call or indicating it's within a script).
      
      Impact:
      - Plain commands, as well as scripts and RM_Call now include the user name.
      - ACL DRYRUN remains the only one that's verbose (mentions the offending channel or key name)
      - Changes RM_Call ACL errors from being a `-ERR` to being `-NOPERM` (besides for textual changes)
        **This somewhat a breaking change, but it only affects the RM_Call with both `C` and `E`, or `D`**
      - Changes ACL errors in scripts textually from being
        `The user executing the script <old non unified text>`
        to
        `ACL failure in script: <new unified text>`
      3193f086
    • Meir Shpilraien (Spielrein)'s avatar
      Fix wrong replication on cluster slotmap changes with module KSN propagation (#11377) · 56f97bfa
      Meir Shpilraien (Spielrein) authored
      As discussed on #11084, `propagatePendingCommands` should happened after the del
      notification is fired so that the notification effect and the `del` will be replicated inside MULTI EXEC.
      
      Test was added to verify the fix.
      56f97bfa
  23. 12 Oct, 2022 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Fix crash on RM_Call inside module load (#11346) · eb6accad
      Meir Shpilraien (Spielrein) authored
      PR #9320 introduces initialization order changes. Now cluster is initialized after modules.
      This changes causes a crash if the module uses RM_Call inside the load function
      on cluster mode (the code will try to access `server.cluster` which at this point is NULL).
      
      To solve it, separate cluster initialization into 2 phases:
      1. Structure initialization that happened before the modules initialization
      2. Listener initialization that happened after.
      
      Test was added to verify the fix.
      eb6accad
  24. 09 Oct, 2022 1 commit
  25. 28 Sep, 2022 1 commit
    • guybe7's avatar
      RM_CreateCommand should not set CMD_KEY_VARIABLE_FLAGS automatically (#11320) · 3330ea18
      guybe7 authored
      The original idea behind auto-setting the default (first,last,step) spec was to use
      the most "open" flags when the user didn't provide any key-spec flags information.
      
      While the above idea is a good approach, it really makes no sense to set
      CMD_KEY_VARIABLE_FLAGS if the user didn't provide the getkeys-api flag:
      in this case there's not way to retrieve these variable flags, so what's the point?
      
      Internally in redis there was code to ignore this already, so this fix doesn't change
      redis's behavior, it only affects the output of COMMAND command.
      3330ea18
  26. 26 Sep, 2022 1 commit
    • Ozan Tezcan's avatar
      Ignore RM_Call deny-oom flag if maxmemory is zero (#11319) · 18920813
      Ozan Tezcan authored
      If a command gets an OOM response and then if we set maxmemory to zero
      to disable the limit, server.pre_command_oom_state never gets updated
      and it stays true. As RM_Call() calls with "respect deny-oom" flag checks
      server.pre_command_oom_state, all calls will fail with OOM.
      
      Added server.maxmemory check in RM_Call() to process deny-oom flag
      only if maxmemory is configured.
      18920813
  27. 22 Sep, 2022 1 commit
    • Shaya Potter's avatar
      Add RM_SetContextUser to support acl validation in RM_Call (and scripts) (#10966) · 6e993a5d
      Shaya Potter authored
      Adds a number of user management/ACL validaiton/command execution functions to improve a
      Redis module's ability to enforce ACLs correctly and easily.
      
      * RM_SetContextUser - sets a RedisModuleUser on the context, which RM_Call will use to both
        validate ACLs (if requested and set) as well as assign to the client so that scripts executed via
        RM_Call will have proper ACL validation.
      * RM_SetModuleUserACLString - Enables one to pass an entire ACL string, not just a single OP
        and have it applied to the user
      * RM_GetModuleUserACLString - returns a stringified version of the user's ACL (same format as dump
        and list).  Contains an optimization to cache the stringified version until the underlying ACL is modified.
      * Slightly re-purpose the "C" flag to RM_Call from just being about ACL check before calling the
        command, to actually running the command with the right user, so that it also affects commands
        inside EVAL scripts. see #11231
      6e993a5d
  28. 21 Sep, 2022 1 commit
  29. 05 Sep, 2022 1 commit
    • Shaya Potter's avatar
      Add a dry run flag to RM_Call execution (#11158) · 87e7973c
      Shaya Potter authored
      Add a new "D" flag to RM_Call which runs whatever verification the user requests,
      but returns before the actual execution of the command.
      
      It automatically enables returning error messages as CallReply objects to distinguish
      success (NULL) from failure (CallReply returned).
      87e7973c
  30. 28 Aug, 2022 1 commit
    • Shaya Potter's avatar
      Improve cmd_flags for script/functions in RM_Call (#11159) · bed6d759
      Shaya Potter authored
      When RM_Call was used with `M` (reject OOM), `W` (reject writes),
      as well as `S` (rejecting stale or write commands in "Script mode"),
      it would have only checked the command flags, but not the declared
      script flag in case it's a command that runs a script.
      
      Refactoring: extracts out similar code in server.c's processCommand
      to be usable in RM_Call as well.
      bed6d759
  31. 24 Aug, 2022 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Reverts most of the changes of #10969 (#11178) · c1bd61a4
      Meir Shpilraien (Spielrein) authored
      The PR reverts the changes made on #10969.
      The reason for revert was trigger because of occasional test failure
      that started after the PR was merged.
      
      The issue is that if there is a lazy expire during the command invocation,
      the `del` command is added to the replication stream after the command
      placeholder. So the logical order on the primary is:
      
      * Delete the key (lazy expiration)
      * Command invocation
      
      But the replication stream gets it the other way around:
      
      * Command invocation (because the command is written into the placeholder)
      * Delete the key (lazy expiration)
      
      So if the command write to the key that was just lazy expired we will get
      inconsistency between primary and replica.
      
      One solution we considered is to add another lazy expire replication stream
      and write all the lazy expire there. Then when replicating, we will replicate the
      lazy expire replication stream first. This will solve this specific test failure but
      we realize that the issues does not ends here and the more we dig the more
      problems we find.One of the example we thought about (that can actually
      crashes Redis) is as follow:
      
      * User perform SINTERSTORE
      * When Redis tries to fetch the second input key it triggers lazy expire
      * The lazy expire trigger a module logic that deletes the first input key
      * Now Redis hold the robj of the first input key that was actually freed
      
      We believe we took the wrong approach and we will come up with another
      PR that solve the problem differently, for now we revert the changes so we
      will not have the tests failure.
      
      Notice that not the entire code was revert, some parts of the PR are changes
      that we would like to keep. The changes that **was** reverted are:
      
      * Saving a placeholder for replication at the beginning of the command (`call` function)
      * Order of the replication stream on active expire and eviction (we will decide how
        to handle it correctly on follow up PR)
      * `Spop` changes are no longer needed (because we reverted the placeholder code)
      
      Changes that **was not** reverted:
      
      * On expire/eviction, wrap the `del` and the notification effect in a multi exec.
      * `PropagateNow` function can still accept a special dbid, -1, indicating not to replicate select.
      * Keep optimisation for reusing the `alsoPropagate` array instead of allocating it each time.
      
      Tests:
      
      * All tests was kept and only few tests was modify to work correctly with the changes
      * Test was added to verify that the revert fixes the issues.
      c1bd61a4
  32. 23 Aug, 2022 1 commit
    • Oran Agra's avatar
      Build TLS as a loadable module · 4faddf18
      Oran Agra authored
      
      
      * Support BUILD_TLS=module to be loaded as a module via config file or
        command line. e.g. redis-server --loadmodule redis-tls.so
      * Updates to redismodule.h to allow it to be used side by side with
        server.h by defining REDISMODULE_CORE_MODULE
      * Changes to server.h, redismodule.h and module.c to avoid repeated
        type declarations (gcc 4.8 doesn't like these)
      * Add a mechanism for non-ABI neutral modules (ones who include
        server.h) to refuse loading if they detect not being built together with
        redis (release.c)
      * Fix wrong signature of RedisModuleDefragFunc, this could break
        compilation of a module, but not the ABI
      * Move initialization of listeners in server.c to be after loading
        the modules
      * Config TLS after initialization of listeners
      * Init cluster after initialization of listeners
      * Add TLS module to CI
      * Fix a test suite race conditions:
        Now that the listeners are initialized later, it's not sufficient to
        wait for the PID message in the log, we need to wait for the "Server
        Initialized" message.
      * Fix issues with moduleconfigs test as a result from start_server
        waiting for "Server Initialized"
      * Fix issues with modules/infra test as a result of an additional module
        present
      
      Notes about Sentinel:
      Sentinel can't really rely on the tls module, since it uses hiredis to
      initiate connections and depends on OpenSSL (won't be able to use any
      other connection modules for that), so it was decided that when TLS is
      built as a module, sentinel does not support TLS at all.
      This means that it keeps using redis_tls_ctx and redis_tls_client_ctx directly.
      
      Example code of config in redis-tls.so(may be use in the future):
      RedisModuleString *tls_cfg = NULL;
      
      void tlsInfo(RedisModuleInfoCtx *ctx, int for_crash_report) {
          UNUSED(for_crash_report);
          RedisModule_InfoAddSection(ctx, "");
          RedisModule_InfoAddFieldLongLong(ctx, "var", 42);
      }
      
      int tlsCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
      {
          if (argc != 2) return RedisModule_WrongArity(ctx);
          return RedisModule_ReplyWithString(ctx, argv[1]);
      }
      
      RedisModuleString *getStringConfigCommand(const char *name, void *privdata) {
          REDISMODULE_NOT_USED(name);
          REDISMODULE_NOT_USED(privdata);
          return tls_cfg;
      }
      
      int setStringConfigCommand(const char *name, RedisModuleString *new, void *privdata, RedisModuleString **err) {
          REDISMODULE_NOT_USED(name);
          REDISMODULE_NOT_USED(err);
          REDISMODULE_NOT_USED(privdata);
          if (tls_cfg) RedisModule_FreeString(NULL, tls_cfg);
          RedisModule_RetainString(NULL, new);
          tls_cfg = new;
          return REDISMODULE_OK;
      }
      
      int RedisModule_OnLoad(void *ctx, RedisModuleString **argv, int argc)
      {
          ....
          if (RedisModule_CreateCommand(ctx,"tls",tlsCommand,"",0,0,0) == REDISMODULE_ERR)
              return REDISMODULE_ERR;
      
          if (RedisModule_RegisterStringConfig(ctx, "cfg", "", REDISMODULE_CONFIG_DEFAULT, getStringConfigCommand, setStringConfigCommand, NULL, NULL) == REDISMODULE_ERR)
              return REDISMODULE_ERR;
      
          if (RedisModule_LoadConfigs(ctx) == REDISMODULE_ERR) {
              if (tls_cfg) {
                  RedisModule_FreeString(ctx, tls_cfg);
                  tls_cfg = NULL;
              }
              return REDISMODULE_ERR;
          }
          ...
      }
      Co-authored-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      Signed-off-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      4faddf18
  33. 18 Aug, 2022 2 commits
    • Binbin's avatar
      Fix memory leak in moduleFreeCommand (#11147) · fc3956e8
      Binbin authored
      Currently, we call zfree(cmd->args), but the argument array
      needs to be freed recursively (there might be sub-args).
      Also fixed memory leaks on cmd->tips and cmd->history.
      
      Fixes #11145
      fc3956e8
    • Meir Shpilraien (Spielrein)'s avatar
      Fix replication inconsistency on modules that uses key space notifications (#10969) · 508a1388
      Meir Shpilraien (Spielrein) authored
      Fix replication inconsistency on modules that uses key space notifications.
      
      ### The Problem
      
      In general, key space notifications are invoked after the command logic was
      executed (this is not always the case, we will discuss later about specific
      command that do not follow this rules). For example, the `set x 1` will trigger
      a `set` notification that will be invoked after the `set` logic was performed, so
      if the notification logic will try to fetch `x`, it will see the new data that was written.
      Consider the scenario on which the notification logic performs some write
      commands. for example, the notification logic increase some counter,
      `incr x{counter}`, indicating how many times `x` was changed.
      The logical order by which the logic was executed is has follow:
      
      ```
      set x 1
      incr x{counter}
      ```
      
      The issue is that the `set x 1` command is added to the replication buffer
      at the end of the command invocation (specifically after the key space
      notification logic was invoked and performed the `incr` command).
      The replication/aof sees the commands in the wrong order:
      
      ```
      incr x{counter}
      set x 1
      ```
      
      In this specific example the order is less important.
      But if, for example, the notification would have deleted `x` then we would
      end up with primary-replica inconsistency.
      
      ### The Solution
      
      Put the command that cause the notification in its rightful place. In the
      above example, the `set x 1` command logic was executed before the
      notification logic, so it should be added to the replication buffer before
      the commands that is invoked by the notification logic. To achieve this,
      without a major code refactoring, we save a placeholder in the replication
      buffer, when finishing invoking the command logic we check if the command
      need to be replicated, and if it does, we use the placeholder to add it to the
      replication buffer instead of appending it to the end.
      
      To be efficient and not allocating memory on each command to save the
      placeholder, the replication buffer array was modified to reuse memory
      (instead of allocating it each time we want to replicate commands).
      Also, to avoid saving a placeholder when not needed, we do it only for
      WRITE or MAY_REPLICATE commands.
      
      #### Additional Fixes
      
      * Expire and Eviction notifications:
        * Expire/Eviction logical order was to first perform the Expire/Eviction
          and then the notification logic. The replication buffer got this in the
          other way around (first notification effect and then the `del` command).
          The PR fixes this issue.
        * The notification effect and the `del` command was not wrap with
          `multi-exec` (if needed). The PR also fix this issue.
      * SPOP command:
        * On spop, the `spop` notification was fired before the command logic
          was executed. The change in this PR would have cause the replication
          order to be change (first `spop` command and then notification `logic`)
          although the logical order is first the notification logic and then the
          `spop` logic. The right fix would have been to move the notification to
          be fired after the command was executed (like all the other commands),
          but this can be considered a breaking change. To overcome this, the PR
          keeps the current behavior and changes the `spop` code to keep the right
          logical order when pushing commands to the replication buffer. Another PR
          will follow to fix the SPOP properly and match it to the other command (we
          split it to 2 separate PR's so it will be easy to cherry-pick this PR to 7.0 if
          we chose to).
      
      #### Unhanded Known Limitations
      
      * key miss event:
        * On key miss event, if a module performed some write command on the
          event (using `RM_Call`), the `dirty` counter would increase and the read
          command that cause the key miss event would be replicated to the replication
          and aof. This problem can also happened on a write command that open
          some keys but eventually decides not to perform any action. We decided
          not to handle this problem on this PR because the solution is complex
          and will cause additional risks in case we will want to cherry-pick this PR.
          We should decide if we want to handle it in future PR's. For now, modules
          writers is advice not to perform any write commands on key miss event.
      
      #### Testing
      
      * We already have tests to cover cases where a notification is invoking write
        commands that are also added to the replication buffer, the tests was modified
        to verify that the replica gets the command in the correct logical order.
      * Test was added to verify that `spop` behavior was kept unchanged.
      * Test was added to verify key miss event behave as expected.
      * Test was added to verify the changes do not break lazy expiration.
      
      #### Additional Changes
      
      * `propagateNow` function can accept a special dbid, -1, indicating not
        to replicate `select`. We use this to replicate `multi/exec` on `propagatePendingCommands`
        function. The side effect of this change is that now the `select` command
        will appear inside the `multi/exec` block on the replication stream (instead of
        outside of the `multi/exec` block). Tests was modified to match this new behavior.
      508a1388
  34. 19 Jul, 2022 1 commit
    • Binbin's avatar
      Fix timing issue in cluster test (#11008) · 5ce64ab0
      Binbin authored
      A timing issue like this was reported in freebsd daily CI:
      ```
      *** [err]: Sanity test push cmd after resharding in tests/unit/cluster/cli.tcl
      Expected 'CLUSTERDOWN The cluster is down' to match '*MOVED*'
      ```
      
      We additionally wait for each node to reach a consensus on the cluster
      state in wait_for_condition to avoid the cluster down error.
      
      The fix just like #10495, quoting madolson's comment:
      Cluster check just verifies the the config state is self-consistent,
      waiting for cluster_state to be okay is an independent check that all
      the nodes actually believe each other are healthy.
      
      At the same time i noticed that unit/moduleapi/cluster.tcl has an exact
      same test, may have the same problem, also modified it.
      5ce64ab0
  35. 12 Jul, 2022 1 commit
  36. 26 Jun, 2022 1 commit