1. 21 Jul, 2021 18 commits
    • Oran Agra's avatar
      Fix failing basics moduleapi test on 32bit CI (#9140) · 1d7c0e59
      Oran Agra authored
      
      (cherry picked from commit 5ffdbae1)
      1d7c0e59
    • Oran Agra's avatar
      Adjustments to recent RM_StringTruncate fix (#3718) (#9125) · 37b0f361
      Oran Agra authored
      - Introduce a new sdssubstr api as a building block for sdsrange.
        The API of sdsrange is many times hard to work with and also has
        corner case that cause bugs. sdsrange is easy to work with and also
        simplifies the implementation of sdsrange.
      - Revert the fix to RM_StringTruncate and just use sdssubstr instead of
        sdsrange.
      - Solve valgrind warnings from the new tests introduced by the previous
        PR.
      
      (cherry picked from commit ae418eca)
      37b0f361
    • Binbin's avatar
      SMOVE only notify dstset when the addition is successful. (#9244) · b6225371
      Binbin authored
      in case dest key already contains the member, the dest key isn't modified, so the command shouldn't invalidate watch.
      
      (cherry picked from commit 11dc4e59)
      b6225371
    • Oran Agra's avatar
      fix valgrind issues with recently added test in modules/blockonbackground (#9192) · de1b19ea
      Oran Agra authored
      fixes test issue introduced in #9167
      
      1. invalid reads due to accessing non-retained string (passed as unblock context).
      2. leaking module blocked client context, see #6922 for info.
      
      (cherry picked from commit a8518cce)
      de1b19ea
    • Yossi Gottlieb's avatar
      Fix CLIENT UNBLOCK crashing modules. (#9167) · 79fa5618
      Yossi Gottlieb authored
      Modules that use background threads with thread safe contexts are likely
      to use RM_BlockClient() without a timeout function, because they do not
      set up a timeout.
      
      Before this commit, `CLIENT UNBLOCK` would result with a crash as the
      `NULL` timeout callback is called. Beyond just crashing, this is also
      logically wrong as it may throw the module into an unexpected client
      state.
      
      This commits makes `CLIENT UNBLOCK` on such clients behave the same as
      any other client that is not in a blocked state and therefore cannot be
      unblocked.
      
      (cherry picked from commit aa139e2f)
      79fa5618
    • Binbin's avatar
      Fix accidental deletion of sinterstore command when we meet wrong type error. (#9032) · 88655019
      Binbin authored
      SINTERSTORE would have deleted the dest key right away,
      even when later on it is bound to fail on an (WRONGTYPE) error.
      
      With this change it first picks up all the input keys, and only later
      delete the dest key if one is empty.
      
      Also add more tests for some commands.
      Mainly focus on
      - `wrong type error`:
      	expand test case (base on sinter bug) in non-store variant
      	add tests for store variant (although it exists in non-store variant, i think it would be better to have same tests)
      - the dstkey result when we meet `non-exist key (empty set)` in *store
      
      sdiff:
      - improve test case about wrong type error (the one we found in sinter, although it is safe in sdiff)
      - add test about using non-exist key (treat it like an empty set)
      sdiffstore:
      - according to sdiff test case, also add some tests about `wrong type error` and `non-exist key`
      - the different is that in sdiffstore, we will consider the `dstkey` result
      
      sunion/sunionstore add more tests (same as above)
      
      sinter/sinterstore also same as above ...
      
      (cherry picked from commit b8a5da80)
      88655019
    • Jason Elbaum's avatar
      Change return value type for ZPOPMAX/MIN in RESP3 (#8981) · fad44611
      Jason Elbaum authored
      When using RESP3, ZPOPMAX/ZPOPMIN should return nested arrays for consistency
      with other commands (e.g. ZRANGE).
      
      We do that only when COUNT argument is present (similarly to how LPOP behaves).
      for reasoning see https://github.com/redis/redis/issues/8824#issuecomment-855427955
      
      This is a breaking change only when RESP3 is used, and COUNT argument is present!
      
      (cherry picked from commit 7f342020)
      fad44611
    • Mikhail Fesenko's avatar
      Direct redis-cli repl prints to stderr, because --rdb can print to stdout.... · 88849712
      Mikhail Fesenko authored
      
      Direct redis-cli repl prints to stderr, because --rdb can print to stdout. fflush stdout after responses  (#9136)
      
      1. redis-cli can output --rdb data to stdout
         but redis-cli also write some messages to stdout which will mess up the rdb.
      
      2. Make redis-cli flush stdout when printing a reply
        This was needed in order to fix a hung in redis-cli test that uses
        --replica.
         Note that printf does flush when there's a newline, but fwrite does not.
      
      3. fix the redis-cli --replica test which used to pass previously
         because it didn't really care what it read, and because redis-cli
         used printf to print these other things to stdout.
      
      4. improve redis-cli --replica test to run with both diskless and disk-based.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarViktor Söderqvist <viktor@zuiderkwast.se>
      (cherry picked from commit 1eb4baa5)
      88849712
    • Oran Agra's avatar
      Test infra, handle RESP3 attributes and big-numbers and bools (#9235) · 6cd84b64
      Oran Agra authored
      - promote the code in DEBUG PROTOCOL to addReplyBigNum
      - DEBUG PROTOCOL ATTRIB skips the attribute when client is RESP2
      - networking.c addReply for push and attributes generate assertion when
        called on a RESP2 client, anything else would produce a broken
        protocol that clients can't handle.
      
      (cherry picked from commit 6a5bac30)
      6cd84b64
    • Binbin's avatar
      hrandfield and zrandmember with count should return emptyarray when key does not exist. (#9178) · c6b3966d
      Binbin authored
      
      
      due to a copy-paste bug, it used to reply with null response rather than empty array.
      this commit includes new tests that are looking at the RESP response directly in
      order to be able to tell the difference between them.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit a418a2d3)
      c6b3966d
    • Oran Agra's avatar
      Tests: add a way to read raw RESP protocol reponses (#9193) · 432e0566
      Oran Agra authored
      This makes it possible to distinguish between null response and an empty
      array (currently the tests infra translates both to an empty string/list)
      
      (cherry picked from commit 7103367a)
      432e0566
    • Leibale Eidelman's avatar
      fix ZRANGESTORE - should return 0 when src points to an empty key (#9089) · 095a6e59
      Leibale Eidelman authored
      
      
      mistakenly it used to return an empty array rather than 0.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit 95274f1f)
      095a6e59
    • Binbin's avatar
      ZRANDMEMBER WITHSCORES with negative COUNT may return bad score (#9162) · 11235c0e
      Binbin authored
      Return a bad score when used with negative count (or count of 1), and non-ziplist encoded zset.
      Also add test to validate the return value and cover the issue.
      
      (cherry picked from commit 4bc5a832)
      11235c0e
    • Evan's avatar
      modules: Add newlen == 0 handling to RM_StringTruncate (#3717) (#3718) · 406caa5f
      Evan authored
      Previously, passing 0 for newlen would not truncate the string at all.
      This adds handling of this case, freeing the old string and creating a new empty string.
      
      Other changes:
      - Move `src/modules/testmodule.c` to `tests/modules/basics.c`
      - Introduce that basic test into the test suite
      - Add tests to cover StringTruncate
      - Add `test-modules` build target for the main makefile
      - Extend `distclean` build target to clean modules too
      
      (cherry picked from commit 1ccf2ca2)
      406caa5f
    • Huang Zhw's avatar
      Fix XTRIM or XADD with LIMIT may delete more entries than Count. (#9048) · e6df2f62
      Huang Zhw authored
      The decision to stop trimming due to LIMIT in XADD and XTRIM was after the limit was reached.
      i.e. the code was deleting **at least** that count of records (from the LIMIT argument's perspective, not the MAXLEN),
      instead of **up to** that count of records.
      see #9046
      
      (cherry picked from commit eaa7a7bb)
      e6df2f62
    • YaacovHazan's avatar
      stabilize tests that involved with load handlers (#8967) · ff272176
      YaacovHazan authored
      When test stop 'load handler' by killing the process that generating the load,
      some commands that already in the input buffer, still might be processed by the server.
      This may cause some instability in tests, that count on that no more commands
      processed after we stop the `load handler'
      
      In this commit, new proc 'wait_load_handlers_disconnected' added, to verify that no more
      cammands from any 'load handler' prossesed, by checking that the clients who
      genreate the load is disconnceted.
      
      Also, replacing check of dbsize with wait_for_ofs_sync before comparing debug digest, as
      it would fail in case the last key the workload wrote was an overridden key (not a new one).
      
      Affected tests
      Race fix:
      - failover command to specific replica works
      - Connect multiple replicas at the same time (issue #141), master diskless=$mdl, replica diskless=$sdl
      - AOF rewrite during write load: RDB preamble=$rdbpre
      
      Cleanup and speedup:
      - Test replication with blocking lists and sorted sets operations
      - Test replication with parallel clients writing in different DBs
      - Test replication partial resync: $descr (diskless: $mdl, $sdl, reconnect: $reconnect
      
      (cherry picked from commit 32a2584e)
      ff272176
    • perryitay's avatar
      Fail EXEC command in case a watched key is expired (#9194) · 3f4f9b63
      perryitay authored
      
      
      There are two issues fixed in this commit: 
      1. we want to fail the EXEC command in case there is a watched key that's logically
         expired but not yet deleted by active expire or lazy expire.
      2. we saw that currently cache time is update in every `call()` (including nested calls),
         this time is being also being use for the isKeyExpired comparison, we want to update
         the cache time only in the first call (execCommand)
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit ac8b1df8)
      3f4f9b63
    • Oran Agra's avatar
      Fix race in client side tracking (#9116) · abd44c83
      Oran Agra authored
      The `Tracking gets notification of expired keys` test in tracking.tcl
      used to hung in valgrind CI quite a lot.
      
      It turns out the reason is that with valgrind and a busy machine, the
      server cron active expire cycle could easily run in the same event loop
      as the command that created `mykey`, so that when they key got expired,
      there were two change events to broadcast, one that set the key and one
      that expired it, but since we used raxTryInsert, the client that was
      associated with the "last" change was the one that created the key, so
      the NOLOOP filtered that event.
      
      This commit adds a test that reproduces the problem by using lazy expire
      in a multi-exec which makes sure the key expires in the same event loop
      as the one that added it.
      
      (cherry picked from commit 9b564b52)
      abd44c83
  2. 01 Jun, 2021 3 commits
  3. 03 May, 2021 3 commits
  4. 19 Apr, 2021 4 commits
    • Hanna Fadida's avatar
      Modules: adding a module type for key space notification (#8759) · 53a4d6c3
      Hanna Fadida authored
      Adding a new type mask ​for key space notification, REDISMODULE_NOTIFY_MODULE, to enable unique notifications from commands on REDISMODULE_KEYTYPE_MODULE type keys (which is currently unsupported).
      
      Modules can subscribe to a module key keyspace notification by RM_SubscribeToKeyspaceEvents,
      and clients by notify-keyspace-events of redis.conf or via the CONFIG SET, with the characters 'd' or 'A' 
      (REDISMODULE_NOTIFY_MODULE type mask is part of the '**A**ll' notation for key space notifications).
      
      Refactor: move some pubsub test infra from pubsub.tcl to util.tcl to be re-used by other tests.
      53a4d6c3
    • guybe7's avatar
      Modules: Replicate lazy-expire even if replication is not allowed (#8816) · f40ca9cb
      guybe7 authored
      Before this commit using RM_Call without "!" could cause the master
      to lazy-expire a key (delete it) but without replicating to replicas.
      This could cause the replica's memory usage to gradually grow and
      could also cause consistency issues if the master and replica have
      a clock diff.
      This bug was introduced in #8617
      
      Added a test which demonstrates that scenario.
      f40ca9cb
    • Harkrishn Patro's avatar
      ACL channels permission handling for save/load scenario. (#8794) · 7a3d1487
      Harkrishn Patro authored
      
      
      In the initial release of Redis 6.2 setting a user to only allow pubsub access to
      a specific channel, and doing ACL SAVE, resulted in an assertion when
      ACL LOAD was used. This was later changed by #8723 (not yet released),
      but still not properly resolved (now it errors instead of crash).
      
      The problem is that the server that generates an ACL file, doesn't know what
      would be the setting of the acl-pubsub-default config in the server that will load it.
      so ACL SAVE needs to always start with resetchannels directive.
      
      This should still be compatible with old acl files (from redis 6.0), and ones from earlier
      versions of 6.2 that didn't mess with channels.
      Co-authored-by: default avatarHarkrishn Patro <harkrisp@amazon.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      7a3d1487
    • sundb's avatar
      Fix ouput buffer limit test (#8803) · 3a955d9a
      sundb authored
      The tail size of c->reply is 16kb, but in the test only publish a
      few chars each time, due to a change in #8699, the obuf limit
      is now checked a new memory allocation is made, so this test
      would have sometimes failed to trigger a soft limit disconnection
      in time.
      
      The solution is to write bigger payloads to the output buffer, but
      still limit their rate (not more than 100k/s).
      3a955d9a
  5. 18 Apr, 2021 3 commits
    • Yossi Gottlieb's avatar
      Revert cluster slot migration tests. (#8806) · c0f5c678
      Yossi Gottlieb authored
      Disables #8649 and subsequent attempts to stabilize the test.
      c0f5c678
    • Oran Agra's avatar
      Fix timing of new replication test (#8807) · a9897b00
      Oran Agra authored
      In github actions CI with valgrind, i saw that even the fast replica
      (one that wasn't paused), didn't get to complete the replication fast
      enough, and ended up getting disconnected by timeout.
      
      Additionally, due to a typo in uname, we didn't get to actually run the
      CPU efficiency part of the test.
      a9897b00
    • Oran Agra's avatar
      Improve testsuite print of log file (#8805) · f4b5a4d8
      Oran Agra authored
      1. the `dump_logs` option would have printed only logs of servers that were
         spawn before the test proc started, and not ones that the test proc
         started inside it.
      2. when a server proc catches an exception it should normally forward the
         exception upwards, specifically when it's an assertion that should be
         caught by a test proc above. however, in `durable` mode, we caught all
         exceptions printed them to stdout and let the code continue,
         this was wrong to do for assertions, which should have still been
         propagated to the test function.
      3. don't bother to search for crash log to print if we printed the the
         entire log anyway
      4. if no crash log was found, no need to print anything (i.e. the fact it
         wasn't found)
      5. rename warnings_from_file to crashlog_from_file
      f4b5a4d8
  6. 15 Apr, 2021 1 commit
    • guybe7's avatar
      Add a timeout mechanism for replicas stuck in fullsync (#8762) · d63d0260
      guybe7 authored
      Starting redis 6.0 (part of the TLS feature), diskless master uses pipe from the fork
      child so that the parent is the one sending data to the replicas.
      This mechanism has an issue in which a hung replica will cause the master to wait
      for it to read the data sent to it forever, thus preventing the fork child from terminating
      and preventing the creations of any other forks.
      
      This PR adds a timeout mechanism, much like the ACK-based timeout,
      we disconnect replicas that aren't reading the RDB file fast enough.
      d63d0260
  7. 14 Apr, 2021 1 commit
  8. 13 Apr, 2021 2 commits
  9. 12 Apr, 2021 1 commit
    • Yossi Gottlieb's avatar
      Fix failing cluster tests. (#8763) · 5e3a15ae
      Yossi Gottlieb authored
      Disable replica migration to avoid a race condition where the
      migrated-from node turns into a replica.
      
      Long term, this test should probably be improved to handle multiple
      slots and accept such auto migrations but this is a quick fix to
      stabilize the CI without completely dropping this test.
      5e3a15ae
  10. 07 Apr, 2021 1 commit
  11. 06 Apr, 2021 2 commits
  12. 05 Apr, 2021 1 commit
    • Huang Zhw's avatar
      Fix "default" and overwritten / reset users will not have pubsub channels... · 3b74b550
      Huang Zhw authored
      Fix "default" and overwritten / reset users will not have pubsub channels permissions by default. (#8723)
      
      Background:
      Redis 6.2 added ACL control for pubsub channels (#7993), which were supposed
      to be permissive by default to retain compatibility with redis 6.0 ACL. 
      But due to a bug, only newly created users got this `acl-pubsub-default` applied,
      while overwritten (updated) users got reset to `resetchannels` (denied).
      
      Since the "default" user exists before loading the config file,
      any ACL change to it, results in an update / overwrite.
      
      So when a "default" user is loaded from config file or include ACL
      file with no channels related rules, the user will not have any
      permissions to any channels. But other users will have default
      permissions to any channels.
      
      When upgraded from 6.0 with config rewrite, this will lead to
      "default" user channels permissions lost.
      When users are loaded from include file, then call "acl load", users
      will also lost channels permissions.
      
      Similarly, the `reset` ACL rule, would have reset the user to be denied
      access to any channels, ignoring `acl-pubsub-default` and breaking
      compatibility with redis 6.0.
      
      The implication of this fix is that it regains compatibility with redis 6.0,
      but breaks compatibility with redis 6.2.0 and 2.0.1. e.g. after the upgrade,
      the default user will regain access to pubsub channels.
      
      Other changes:
      Additionally this commit rename server.acl_pubusub_default to
      server.acl_pubsub_default and fix typo in acl tests.
      3b74b550