1. 30 May, 2022 1 commit
  2. 29 May, 2022 1 commit
    • Binbin's avatar
      Fix sentinel disconnect test timing issue after auth-pass change (#10784) · 1013cbea
      Binbin authored
      There is a timing issue reported in test-sanitizer-address (gcc):
      ```
      Sentinels (re)connection following SENTINEL SET mymaster auth-pass:
      FAILED: Expected to be disconnected from master due to wrong password
      ```
      
      The reason we reach it, is because the test is fast enough to modify
      auth-pass and test sentinel connection status with the server,
      before its scheduled operation got the chance to update connection
      status with the server.
      
      We need to wait for `sentinelTimer` to kick in, and then update the
      connection status. Replace condition with wait_for_condition on the check.
      
      Fix just like #10480 did
      1013cbea
  3. 27 May, 2022 1 commit
    • Vitaly's avatar
      Fix ZRANGESTORE crash when zset_max_listpack_entries is 0 (#10767) · 6461f09f
      Vitaly authored
      When `zrangestore` is called container destination object is created. 
      Before this PR we used to create a listpack based object even if `zset-max-ziplist-entries`
      or equivalent`zset-max-listpack-entries` was set to 0.
      This triggered immediate conversion of the listpack into a skiplist in `zrangestore`, which hits
      an assertion resulting in an engine crash.
      
      Added a TCL test that reproduces this issue.
      6461f09f
  4. 26 May, 2022 1 commit
  5. 25 May, 2022 1 commit
  6. 23 May, 2022 1 commit
    • Binbin's avatar
      Fix BZMPOP gets unblocked by non-key args and returns them (#10764) · 450c88f3
      Binbin authored
      This bug was introduced in #9484 (7.0.0).
      It result that BZMPOP blocked on non-key arguments.
      
      Like `bzmpop 0 1 myzset min count 10`, this command will additionally
      block in these keys (except for the first and the last argument) and can return their values:
      - 0: timeout value
      - 1: numkeys value
      - min: min/max token
      - count: count token
      450c88f3
  7. 22 May, 2022 1 commit
    • Oran Agra's avatar
      Scripts that declare the `no-writes` flag are implicitly `allow-oom` too. (#10699) · b0e18f80
      Oran Agra authored
      Scripts that have the `no-writes` flag, cannot execute write commands,
      and since all `deny-oom` commands are write commands, we now act
      as if the `allow-oom` flag is implicitly set for scripts that set the `no-writes` flag.
      this also implicitly means that the EVAL*_RO and FCALL_RO commands can
      never fails with OOM error.
      
      Note about a bug that's no longer relevant:
      There was an issue with EVAL*_RO using shebang not being blocked correctly
      in OOM state:
      When an EVAL script declares a shebang, it was by default not allowed to run in
      OOM state.
      but this depends on a flag that is updated before the command is executed, which
      was not updated in case of the `_RO` variants.
      the result is that if the previous cached state was outdated (either true or false),
      the script will either unjustly fail with OOM, or unjustly allowed to run despite
      the OOM state.
      It doesn't affect scripts without a shebang since these depend on the actual
      commands they run, and since these are only read commands, they don't care
      for that cached oom state flag.
      it did affect scripts with shebang and no allow-oom flag, bug after the change in
      this PR, scripts that are run with eval_ro would implicitly have that flag so again
      the cached state doesn't matter.
      
      p.s. this isn't a breaking change since all it does is allow scripts to run when they
      should / could rather than blocking them.
      b0e18f80
  8. 13 May, 2022 1 commit
    • Wen Hui's avatar
      Update comments on command args, and a misleading error reply (#10645) · 135998ed
      Wen Hui authored
      Updated the comments for:
      info command
      lmpopCommand and blmpopCommand
      sinterGenericCommand 
      
      Fix the missing "key" words in the srandmemberCommand function
      For LPOS command, when rank is 0, prompt user that rank could be
      positive number or negative number, and add a test for it
      135998ed
  9. 12 May, 2022 1 commit
    • Binbin's avatar
      Fix race in module fork kill test (#10717) · 586a16ad
      Binbin authored
      The purpose of the test is to kill the child while it is running.
      From the last two lines we can see the child exits before being killed.
      ```
      - Module fork started pid: 56998
      * <fork> fork child started
      - Killing running module fork child: 56998
      * <fork> fork child exiting
      signal-handler (1652267501) Received SIGUSR1 in child, exiting now.
      ```
      
      In this commit, we pass an argument to `fork.create` indicating how
      long it should sleep. For the fork kill test, we use a longer time to
      avoid the child exiting before being killed.
      
      Other changes:
      use wait_for_condition instead of hardcoded `after 250`.
      Unify the test for failing fork with the one for killing it (save time) 
      586a16ad
  10. 11 May, 2022 2 commits
    • Binbin's avatar
      redis-server command line arguments support take one bulk string with spaces... · bfbb15f7
      Binbin authored
      redis-server command line arguments support take one bulk string with spaces for MULTI_ARG configs parsing. And allow options value to use the -- prefix (#10660)
      
      ## Take one bulk string with spaces for MULTI_ARG configs parsing
      Currently redis-server looks for arguments that start with `--`,
      and anything in between them is considered arguments for the config.
      like: `src/redis-server --shutdown-on-sigint nosave force now --port 6380`
      
      MULTI_ARG configs behave differently for CONFIG command, vs the command
      line argument for redis-server.
      i.e. CONFIG command takes one bulk string with spaces in it, while the
      command line takes an argv array with multiple values.
      
      In this PR, in config.c, if `argc > 1` we can take them as is,
      and if the config is a `MULTI_ARG` and `argc == 1`, we will split it by spaces.
      
      So both of these will be the same:
      ```
      redis-server --shutdown-on-sigint nosave force now --shutdown-on-sigterm nosave force
      redis-server --shutdown-on-sigint nosave "force now" --shutdown-on-sigterm nosave force
      redis-server --shutdown-on-sigint nosave "force now" --shutdown-on-sigterm "nosave force"
      ```
      
      ## Allow options value to use the `--` prefix
      Currently it decides to switch to the next config, as soon as it sees `--`, 
      even if there was not a single value provided yet to the last config,
      this makes it impossible to define a config value that has `--` prefix in it.
      
      For instance, if we want to set the logfile to `--my--log--file`,
      like `redis-server --logfile --my--log--file --loglevel verbose`,
      current code will handle that incorrectly.
      
      In this PR, now we allow a config value that has `--` prefix in it.
      **But note that** something like `redis-server --some-config --config-value1 --config-value2 --loglevel debug`
      would not work, because if you want to pass a value to a config starting with `--`, it can only be a single value.
      like: `redis-server --some-config "--config-value1 --config-value2" --loglevel debug`
      
      An example (using `--` prefix config value):
      ```
      redis-server --logfile --my--log--file --loglevel verbose
      redis-cli config get logfile loglevel
      1) "loglevel"
      2) "verbose"
      3) "logfile"
      4) "--my--log--file"
      ```
      
      ### Potentially breaking change
      `redis-server --save --loglevel verbose` used to work the same as `redis-server --save "" --loglevel verbose`
      now, it'll error!
      bfbb15f7
    • Binbin's avatar
      FLUSHDB and FLUSHALL add call forceCommandPropagation / FLUSHALL reset dirty... · 783b210d
      Binbin authored
      FLUSHDB and FLUSHALL add call forceCommandPropagation / FLUSHALL reset dirty counter to 0 if we enable save (#10691)
      
      ## FLUSHALL
      We used to restore the dirty counter after `rdbSave` zeroed it if we enable save.
      Otherwise FLUSHALL will not be replicated nor put into the AOF.
      
      And then we do increment it again below.
      Without that extra dirty++, when db was already empty, FLUSHALL
      will not be replicated nor put into the AOF.
      
      We now gonna replace all that dirty counter magic with a call
      to forceCommandPropagation (REPL and AOF), instead of all the
      messing around with the dirty counter.
      Added tests to cover three part (dirty counter, REPL, AOF).
      
      One benefit other than cleaner code is that the `rdb_changes_since_last_save` is correct in this case.
      
      ## FLUSHDB
      FLUSHDB was not replicated nor put into the AOF when db was already empty.
      Unlike DEL on a non-existing key, FLUSHDB always does something, and that's to call the module hook. 
      So basically FLUSHDB is never a NOP, and thus it should always be propagated.
      Not doing that, could mean that if a module does something in that hook, and wants to
      avoid issues of that hook being missing on the replica if the db is empty, it'll need to do complicated things.
      
      So now FLUSHDB add call forceCommandPropagation, we will always propagate FLUSHDB.
      Always propagating FLUSHDB seems like a safe approach that shouldn't have any drawbacks (other than looking odd)
      
      This was mentioned in #8972
      
      ## Test section:
      We actually found it while solving a race condition in the BGSAVE test (other.tcl).
      It was found in extra_ci Daily Arm64 (test-libc-malloc).
      ```
      [exception]: Executing test client: ERR Background save already in progress.
      ERR Background save already in progress
      ```
      
      It look like `r flushdb` trigger (schedule) a bgsave right after `waitForBgsave r` and before `r save`.
      Changing flushdb to flushall, FLUSHALL will do a foreground save and then set the dirty counter to 0.
      783b210d
  11. 10 May, 2022 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Fix #10705, avoid relinking the same library twice. (#10706) · 442e73ea
      Meir Shpilraien (Spielrein) authored
      Set `old_li` to NULL to avoid linking it again on error.
      Before the fix, loading an already existing library will cause the existing library to be added again. This cause not harm other then wrong statistics. The statistics that are effected  by the issue are:
      * `libraries_count` and `functions_count` returned by `function stats` command
      * `used_memory_functions` returned on `info memory` command
      * `functions.caches` returned on `memory stats` command
      442e73ea
  12. 09 May, 2022 2 commits
  13. 02 May, 2022 1 commit
  14. 26 Apr, 2022 7 commits
    • meir's avatar
      Protect any table which is reachable from globals and added globals white list. · efa162bc
      meir authored
      The white list is done by setting a metatable on the global table before initializing
      any library. The metatable set the `__newindex` field to a function that check
      the white list before adding the field to the table. Fields which is not on the
      white list are simply ignored.
      
      After initialization phase is done we protect the global table and each table
      that might be reachable from the global table. For each table we also protect
      the table metatable if exists.
      efa162bc
    • meir's avatar
      Protect globals of both evals scripts and functions. · 3731580b
      meir authored
      Use the new `lua_enablereadonlytable` Lua API to protect the global tables of
      both evals scripts and functions. For eval scripts, the implemetation is easy,
      We simply call `lua_enablereadonlytable` on the global table to turn it into
      a readonly table.
      
      On functions its more complecated, we want to be able to switch globals between
      load run and function run. To achieve this, we create a new empty table that
      acts as the globals table for function, we control the actual globals using metatable
      manipulation. Notice that even if the user gets a pointer to the original tables, all
      the tables are set to be readonly (using `lua_enablereadonlytable` Lua API) so he can
      not change them. The following inlustration better explain the solution:
      
      ```
      Global table {} <- global table metatable {.__index = __real_globals__}
      ```
      
      The `__real_globals__` is set depends on the run context (function load or function call).
      
      Why this solution is needed and its not enough to simply switch globals?
      When we run in the context of function load and create our functions, our function gets
      the current globals that was set when they were created. Replacing the globals after
      the creation will not effect them. This is why this trick it mandatory.
      3731580b
    • Oran Agra's avatar
      Add module API flag for using enum configs as bit flags (#10643) · 81926254
      Oran Agra authored
      Enables registration of an enum config that'll let the user pass multiple keywords that
      will be combined with `|` as flags into the integer config value.
      
      ```
          const char *enum_vals[] = {"none", "one", "two", "three"};
          const int int_vals[] = {0, 1, 2, 4};
      
          if (RedisModule_RegisterEnumConfig(ctx, "flags", 3, REDISMODULE_CONFIG_DEFAULT | REDISMODULE_CONFIG_BITFLAGS, enum_vals, int_vals, 4, getFlagsConfigCommand, setFlagsConfigCommand, NULL, NULL) == REDISMODULE_ERR) {
              return REDISMODULE_ERR;
          }
      ```
      doing:
      `config set moduleconfigs.flags "two three"` will result in 6 being passed to`setFlagsConfigCommand`.
      81926254
    • chenyang8094's avatar
      Fix bug when AOF enabled after startup. put the new incr file in the manifest... · 46ec6ad9
      chenyang8094 authored
      Fix bug when AOF enabled after startup. put the new incr file in the manifest only when AOFRW is done. (#10616)
      
      Changes:
      
      - When AOF is enabled **after** startup, the data accumulated during `AOF_WAIT_REWRITE`
        will only be stored in a temp INCR AOF file. Only after the first AOFRW is successful, we will
        add it to manifest file.
        Before this fix, the manifest referred to the temp file which could cause a restart during that
        time to load it without it's base.
      - Add `aof_rewrites_consecutive_failures` info field for  aofrw limiting implementation.
      
      Now we can guarantee that these behaviors of MP-AOF are the same as before (past redis releases):
      - When AOF is enabled after startup, the data accumulated during `AOF_WAIT_REWRITE` will only
        be stored in a visible place. Only after the first AOFRW is successful, we will add it to manifest file.
      - When disable AOF, we did not delete the AOF file in the past so there's no need to change that
        behavior now (yet).
      - When toggling AOF off and then on (could be as part of a full-sync), a crash or restart before the
        first rewrite is completed, would result with the previous version being loaded (might not be right thing,
        but that's what we always had).
      46ec6ad9
    • Eduardo Semprebon's avatar
      Allow configuring signaled shutdown flags (#10594) · 3a1d1425
      Eduardo Semprebon authored
      
      
      The SHUTDOWN command has various flags to change it's default behavior,
      but in some cases establishing a connection to redis is complicated and it's easier
      for the management software to use signals. however, so far the signals could only
      trigger the default shutdown behavior.
      Here we introduce the option to control shutdown arguments for SIGTERM and SIGINT.
      
      New config options:
      `shutdown-on-sigint [nosave | save] [now] [force]` 
      `shutdown-on-sigterm [nosave | save] [now] [force]`
      
      Implementation:
      Support MULTI_ARG_CONFIG on createEnumConfig to support multiple enums to be applied as bit flags.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      3a1d1425
    • Madelyn Olson's avatar
      Set replicas to panic on disk errors, and optionally panic on replication errors (#10504) · 6fa8e4f7
      Madelyn Olson authored
      * Till now, replicas that were unable to persist, would still execute the commands
        they got from the master, now they'll panic by default, and we add a new
        `replica-ignore-disk-errors` config to change that.
      * Till now, when a command failed on a replica or AOF-loading, it only logged a
        warning and a stat, we add a new `propagation-error-behavior` config to allow
        panicking in that state (may become the default one day)
      
      Note that commands that fail on the replica can either indicate a bug that could
      cause data inconsistency between the replica and the master, or they could be
      in some cases (specifically in previous versions), a result of a command (e.g. EVAL)
      that failed on the master, but still had to be propagated to fail on the replica as well.
      6fa8e4f7
    • Madelyn Olson's avatar
      By default prevent cross slot operations in functions and scripts with # (#10615) · efcd1bf3
      Madelyn Olson authored
      Adds the `allow-cross-slot-keys` flag to Eval scripts and Functions to allow
      scripts to access keys from multiple slots.
      The default behavior is now that they are not allowed to do that (unlike before).
      This is a breaking change for 7.0 release candidates (to be part of 7.0.0), but
      not for previous redis releases since EVAL without shebang isn't doing this check.
      
      Note that the check is done on both the keys declared by the EVAL / FCALL command
      arguments, and also the ones used by the script when making a `redis.call`.
      
      A note about the implementation, there seems to have been some confusion
      about allowing access to non local keys. I thought I missed something in our
      wider conversation, but Redis scripts do block access to non-local keys.
      So the issue was just about cross slots being accessed.
      efcd1bf3
  15. 25 Apr, 2022 2 commits
    • guybe7's avatar
      Fix regression not aborting transaction on error, and re-edit some error responses (#10612) · df787764
      guybe7 authored
      1. Disk error and slave count checks didn't flag the transactions or counted correctly in command stats (regression from #10372  , 7.0 RC3)
      2. RM_Call will reply the same way Redis does, in case of non-exisitng command or arity error
      3. RM_WrongArtiy will consider the full command name
      4. Use lowercase 'u' in "unknonw subcommand" (to align with "unknown command")
      
      Followup work of #10127
      df787764
    • guybe7's avatar
      Test: RM_Call from within "expired" notification (#10613) · 21e39ec4
      guybe7 authored
      This case is interesting because it originates from cron,
      rather than from another command.
      
      The idea came from looking at #9890 and #10573, and I was wondering if RM_Call
      would work properly when `server.current_client == NULL`
      21e39ec4
  16. 24 Apr, 2022 2 commits
    • Yossi Gottlieb's avatar
      Run large-memory tests as solo. (#10626) · bd823c7f
      Yossi Gottlieb authored
      This avoids random memory spikes and enables --large-memory tests to run
      on moderately sized systems.
      bd823c7f
    • Binbin's avatar
      Fix timing issue in slowlog redact test (#10614) · a6b3ce28
      Binbin authored
      * Fix timing issue in slowlog redact test
      
      This test failed once in my daily CI (test-sanitizer-address (clang))
      ```
      *** [err]: SLOWLOG - Some commands can redact sensitive fields in tests/unit/slowlog.tcl
      Expected 'migrate 127.0.0.1 25649 key 9 5000 AUTH2 (redacted) (redacted)' to match '* key 9 5000 AUTH (redacted)' (context: type eval line 12 cmd {assert_match {* key 9 5000 AUTH (redacted)} [lindex [lindex [r slowlog get] 1] 3]} proc ::test)
      ```
      
      The reason is that with slowlog-log-slower-than 10000,
      slowlog get will have a chance to exceed 10ms.
      
      Change slowlog-log-slower-than from 10000 to -1, disable it.
      Also handles a same potentially problematic test above.
      This is actually the same timing issue as #10432.
      
      But also avoid repeated calls to `SLOWLOG GET`
      a6b3ce28
  17. 20 Apr, 2022 1 commit
  18. 19 Apr, 2022 2 commits
  19. 18 Apr, 2022 1 commit
    • Oran Agra's avatar
      Fix RM_Yield bug processing future commands of the current client. (#10573) · 7d1ad6ca
      Oran Agra authored
      RM_Yield was missing a call to protectClient to prevent redis from
      processing future commands of the yielding client.
      
      Adding tests that fail without this fix.
      
      This would be complicated to solve since nested calls to RM_Call used to
      replace the current_client variable with the module temp client.
      
      It looks like it's no longer necessary to do that, since it was added
      back in #9890 to solve two issues, both already gone:
      1. call to CONFIG SET maxmemory could trigger a module hook calling
         RM_Call. although this specific issue is gone, arguably other hooks
         like keyspace notification, can do the same.
      2. an assertion in lookupKey that checks the current command of the
         current client, introduced in #9572 and removed in #10248
      7d1ad6ca
  20. 17 Apr, 2022 3 commits
    • guybe7's avatar
      Add RM_PublishMessageShard (#10543) · f49ff156
      guybe7 authored
      since PUBLISH and SPUBLISH use different dictionaries for channels and clients,
      and we already have an API for PUBLISH, it only makes sense to have one for SPUBLISH
      
      Add test coverage and unifying some test infrastructure.
      f49ff156
    • Meir Shpilraien (Spielrein)'s avatar
      Added test to verify loading Lua binary payload is not possible (#10583) · 789c94fe
      Meir Shpilraien (Spielrein) authored
      The tests verify that loading a binary payload to the Lua interpreter raises an error.
      The Lua code modification was done here: fdf9d455
      which force the Lau interpreter to always use the text parser.
      789c94fe
    • guybe7's avatar
      Add RM_MallocSizeString, RM_MallocSizeDict (#10542) · fe1c096b
      guybe7 authored
      Add APIs to allow modules to compute the memory consumption of opaque objects owned by redis.
      Without these, the mem_usage callbacks of module data types are useless in many cases.
      
      Other changes:
      Fix streamRadixTreeMemoryUsage to include the size of the rax structure itself
      fe1c096b
  21. 14 Apr, 2022 2 commits
    • Madelyn Olson's avatar
      Fix incorrect error code for eval scripts and fix test error checking (#10575) · effa707e
      Madelyn Olson authored
      By the convention of errors, there is supposed to be a space between the code and the name.
      While looking at some lua stuff I noticed that interpreter errors were not adding the space,
      so some clients will try to map the detailed error message into the error.
      
      We have tests that hit this condition, but they were just checking that the string "starts" with ERR.
      I updated some other tests with similar incorrect string checking. This isn't complete though, as
      there are other ways we check for ERR I didn't fix.
      
      Produces some fun output like:
      ```
      # Errorstats
      errorstat_ERR:count=1
      errorstat_ERRuser_script_1_:count=1
      ```
      effa707e
    • Oran Agra's avatar
      solve corrupt dump fuzzer crash in streams (#10579) · 95050f26
      Oran Agra authored
      we had a panic in streamLastValidID when the stream metadata
      said it's not empty, but the rax is empty.
      95050f26
  22. 13 Apr, 2022 1 commit
    • Luke Palmer's avatar
      Keyspace event for new keys (#10512) · bb7891f0
      Luke Palmer authored
      Add an optional keyspace event when new keys are added to the db.
      
      This is useful for applications where clients need to be aware of the redis keyspace.
      Such an application can SCAN once at startup and then listen for "new" events (plus
      others associated with DEL, RENAME, etc).
      bb7891f0
  23. 12 Apr, 2022 1 commit
  24. 10 Apr, 2022 1 commit
    • guybe7's avatar
      COMMAND DOCS shows module name, where applicable (#10544) · 719db14e
      guybe7 authored
      Add field to COMMAND DOCS response to denote the name of the module
      that added that command.
      COMMAND LIST can filter by module, but if you get the full commands list,
      you may still wanna know which command belongs to which module.
      The alternative would be to do MODULE LIST, and then multiple calls to COMMAND LIST
      719db14e
  25. 07 Apr, 2022 1 commit
    • Oran Agra's avatar
      Fix RM_Yield bug (#10548) · 451531f1
      Oran Agra authored
      The bug was when using REDISMODULE_YIELD_FLAG_CLIENTS.
      in that case we would have only set the CLIENTS type flag in
      server.busy_module_yield_flags and then clear that flag when exiting
      RM_Yield, so we would never call unblockPostponedClients when the
      context is destroyed.
      
      This didn't really have any actual implication, which is why the tests
      couldn't (and still can't) find that since the bug only happens when
      using CLIENT, but in this case we won't have any clients to un-postpone
      i.e. clients will get rejected with BUSY error, rather than being
      postponed.
      
      Unrelated:
      * Adding tests for nested contexts, just in case.
      * Avoid nested RM_Yield calls
      451531f1
  26. 05 Apr, 2022 1 commit
    • Lu JJ's avatar
      Fix the bug that caused hash encoding errors when using hincrbyfloat or hincrby commands (#10479) · f110de4b
      Lu JJ authored
      Fixed a bug that used the `hincrbyfloat` or `hincrby` commands to make the field or value exceed the
      `hash_max_listpack_value` but did not change the object encoding of the hash structure.
      
      Add a length check for field and value, check the length of value first, if the length of value does not
      exceed `hash_max_listpack_value` then check the length of field.
      
      If the length of field or value is too long, it will reduce the efficiency of listpack, and the object encoding
      will become hashtable after AOF restart, so this is also to keep the same before and after AOF restart.
      f110de4b