1. 01 Aug, 2023 1 commit
  2. 30 Jul, 2023 1 commit
  3. 25 Jul, 2023 4 commits
    • Harkrishn Patro's avatar
      Test coverage for incr/decr operation on robj encoding type optimization (#12435) · 42985b00
      Harkrishn Patro authored
      Additional test coverage for incr/decr operation.
      
      integer number could be present in raw encoding format due to operation like append. A incr/decr operation following it optimize the string to int encoding format.
      42985b00
    • zhaozhao.zz's avatar
      update monitor client's memory and evict correctly (#12420) · 01eb939a
      zhaozhao.zz authored
      A bug introduced in #11657 (7.2 RC1), causes client-eviction (#8687)
      and INFO to have inaccurate memory usage metrics of MONITOR clients.
      
      Because the type in `c->type` and the type in `getClientType()` are confusing
      (in the later, `CLIENT_TYPE_NORMAL` not `CLIENT_TYPE_SLAVE`), the comment
      we wrote in `updateClientMemUsageAndBucket` was wrong, and in fact that function
      didn't skip monitor clients.
      And since it doesn't skip monitor clients, it was wrong to delete the call for it from
      `replicationFeedMonitors` (it wasn't a NOP).
      That deletion could mean that the monitor client memory usage is not always up to
      date (updated less frequently, but still a candidate for client eviction).
      01eb939a
    • nihohit's avatar
      Update request/response policies. (#12417) · 9f512017
      nihohit authored
      changing the response and request policy of a few commands,
      see https://redis.io/docs/reference/command-tips
      
      
      
      1. RANDOMKEY used to have no response policy, which means
        that when sent to multiple shards, the responses should be aggregated.
        this normally applies to commands that return arrays, but since RANDOMKEY
        replies with a simple string, it actually requires a SPECIAL response policy
        (for the client to select just one)
      2. SCAN used to have no response policy, but although the key names part of
        the response can be aggregated, the cursor part certainly can't.
      3. MSETNX had a request policy of MULTI_SHARD and response policy of AGG_MIN,
        but in fact the contract with MSETNX is that when one key exists, it returns 0
        and doesn't set any key, routing it to multiple shards would mean that if one failed
        and another succeeded, it's atomicity is broken and it's impossible to return a valid
        response to the caller.
      Co-authored-by: default avatarShachar Langbeheim <shachlan@amazon.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      9f512017
    • Harkrishn Patro's avatar
      Add test case for APPEND command usage on integer value (#12429) · 34b95f75
      Harkrishn Patro authored
      Add test coverage to validate object encoding update on APPEND command usage on a integer value
      34b95f75
  4. 20 Jul, 2023 1 commit
    • Makdon's avatar
      redis-cli: use previous hostip when not provided by redis cluster server (#12273) · 2495b90a
      Makdon authored
      
      
      When the redis server cluster running on cluster-preferred-endpoint-type unknown-endpoint mode, and receive a request that should be redirected to another redis server node, it does not reply the hostip, but a empty host like MOVED 3999 :6381.
      
      The redis-cli would try to connect to an address without a host, which cause the issue:
      ```
      127.0.0.1:7002> set bar bar
      -> Redirected to slot [5061] located at :7000
      Could not connect to Redis at :7000: No address associated with hostname
      Could not connect to Redis at :7000: No address associated with hostname
      not connected> exit
      ```
      
      In this case, the redis-cli should use the previous hostip when there's no host provided by the server.
      
      ---------
      Co-authored-by: default avatarViktor Söderqvist <viktor.soderqvist@est.tech>
      Co-authored-by: default avatarMadelyn Olson <madelynolson@gmail.com>
      2495b90a
  5. 18 Jul, 2023 1 commit
  6. 16 Jul, 2023 2 commits
    • George Guimares's avatar
      Replaced comment with excessive warning. · 9a3b99cb
      George Guimares authored
      The data structures in the comment are not in sync and don't need to be.
      Referring to function that handles conversion.
      9a3b99cb
    • Chen Tianjie's avatar
      Hide the comma after cport when there is no hostname. (#12411) · 91011100
      Chen Tianjie authored
      According to the format shown in https://redis.io/commands/cluster-nodes/
      ```
      <ip:port@cport[,hostname[,auxiliary_field=value]*]>
      ```
      when there is no hostname, and the auxiliary fields are hidden, the cluster topology should be
      ```
      <ip:port@cport>
      ```
      However in the code we always print the hostname even when it is an empty string, leaving an unnecessary comma tailing after cport, which is weird and conflicts with the doc.
      ```
      94ca2f6cf85228a49fde7b738ee1209de7bee325 127.0.0.1:6379@16379, myself,master - 0 0 0 connected 0-16383
      ```
      91011100
  7. 13 Jul, 2023 4 commits
  8. 10 Jul, 2023 1 commit
  9. 07 Jul, 2023 1 commit
    • Binbin's avatar
      Initialize cluster owner_not_claiming_slot to avoid warning (#12391) · 14f802b3
      Binbin authored
      valgrind report a Uninitialised warning:
      ```
      ==25508==  Uninitialised value was created by a heap allocation
      ==25508==    at 0x4848899: malloc (in
      /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
      ==25508==    by 0x1A35A1: ztrymalloc_usable_internal (zmalloc.c:117)
      ==25508==    by 0x1A368D: zmalloc (zmalloc.c:145)
      ==25508==    by 0x21FDEA: clusterInit (cluster.c:973)
      ==25508==    by 0x19DC09: main (server.c:7306)
      ```
      
      Introduced in #12344
      14f802b3
  10. 06 Jul, 2023 2 commits
    • zhaozhao.zz's avatar
      add an assertion to make sure numkeys is 0 in getKeysUsingKeySpecs (#12389) · aefdc57f
      zhaozhao.zz authored
      This is an addition to #12380, to prevent potential bugs when collecting
      keys from multiple commands in the future.
      Note that this function also resets numkeys in some cases.
      aefdc57f
    • Sankar's avatar
      Process loss of slot ownership in cluster bus (#12344) · 1190f25c
      Sankar authored
      Process loss of slot ownership in cluster bus
      
      When a node no longer owns a slot, it clears the bit corresponding
      to the slot in the cluster bus messages. The receiving nodes
      currently don't record the fact that the sender stopped claiming
      a slot until some other node in the cluster starts claiming the slot.
      This can cause a slot to go missing during slot migration when subjected
      to inopportune race with addition of new shards or a failover.
      This fix forces the receiving nodes to process the loss of ownership
      to avoid spreading wrong information.
      1190f25c
  11. 05 Jul, 2023 2 commits
    • Job Henandez Lara's avatar
      redis-benchmark: add documentation for the amount of clients needed (#12372) · b35e20d1
      Job Henandez Lara authored
      In cluster mode, we need more clients than the number of nodes.
      b35e20d1
    • Binbin's avatar
      Fix and increase tollerance of event loop test, add verbose logs (#12385) · d56a7d9b
      Binbin authored
      The test fails on freebsd CI:
      ```
      *** [err]: stats: eventloop metrics in tests/unit/info.tcl
      Expected '31777' to be less than '16183' (context: type eval line 17 cmd
      {assert_lessthan $el_sum2 [expr $el_sum1+10000] } proc ::test)
      ```
      
      The test added in #11963, fails on freebsd CI which is slow,
      increase tollerance and also add some verbose logs, now we can
      see these logs in verbose mode (for better views):
      ```
      eventloop metrics cycle1: 12, cycle2: 15
      eventloop metrics el_sum1: 315, el_sum2: 411
      eventloop metrics cmd_sum1: 126, cmd_sum2: 137
      [ok]: stats: eventloop metrics (111 ms)
      instantaneous metrics instantaneous_eventloop_cycles_per_sec: 8
      instantaneous metrics instantaneous_eventloop_duration_usec: 55
      [ok]: stats: instantaneous metrics (1603 ms)
      [ok]: stats: debug metrics (112 ms)
      ```
      d56a7d9b
  12. 03 Jul, 2023 1 commit
    • Lior Lahav's avatar
      Fix possible crash in command getkeys (#12380) · b7559d9f
      Lior Lahav authored
      
      
      When getKeysUsingKeySpecs processes a command with more than one key-spec,
      and called with a total of more than 256 keys, it'll call getKeysPrepareResult again,
      but since numkeys isn't updated, getKeysPrepareResult will not bother to copy key
      names from the old result (leaving these slots uninitialized). Furthermore, it did not
      consider the keys it already found when allocating more space.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      b7559d9f
  13. 02 Jul, 2023 1 commit
  14. 01 Jul, 2023 1 commit
  15. 29 Jun, 2023 1 commit
  16. 27 Jun, 2023 5 commits
  17. 26 Jun, 2023 1 commit
    • Chen Tianjie's avatar
      Support TLS service when "tls-cluster" is not enabled and persist both plain... · 22a29935
      Chen Tianjie authored
      Support TLS service when "tls-cluster" is not enabled and persist both plain and TLS port in nodes.conf (#12233)
      
      Originally, when "tls-cluster" is enabled, `port` is set to TLS port. In order to support non-TLS clients, `pport` is used to propagate TCP port across cluster nodes. However when "tls-cluster" is disabled, `port` is set to TCP port, and `pport` is not used, which means the cluster cannot provide TLS service unless "tls-cluster" is on.
      ```
      typedef struct {
          // ...
          uint16_t port;  /* Latest known clients port (TLS or plain). */
          uint16_t pport; /* Latest known clients plaintext port. Only used if the main clients port is for TLS. */
          // ...
      } clusterNode;
      ```
      ```
      typedef struct {
          // ...
          uint16_t port;   /* TCP base port number. */
          uint16_t pport;  /* Sender TCP plaintext port, if base port is TLS */
          // ...
      } clusterMsg;
      ```
      This PR renames `port` and `pport` in `clusterNode` to `tcp_port` and `tls_port`, to record both ports no matter "tls-cluster" is enabled or disabled.
      
      This allows to provide TLS service to clients when "tls-cluster" is disabled: when displaying cluster topology, or giving `MOVED` error, server can provide TLS or TCP port according to client's connection type, no matter what type of connection cluster bus is using.
      
      For backwards compatibility, `port` and `pport` in `clusterMsg` are preserved, when "tls-cluster" is enabled, `port` is set to TLS port and `pport` is set to TCP port, when "tls-cluster" is disabled, `port` is set to TCP port and `pport` is set to TLS port (instead of 0).
      
      Also, in the nodes.conf file, a new aux field displaying an extra port is added to complete the persisted info. We may have `tls_port=xxxxx` or `tcp_port=xxxxx` in the aux field, to complete the cluster topology, while the other port is stored in the normal `<ip>:<port>` field. The format is shown below.
      ```
      <node-id> <ip>:<tcp_port>@<cport>,<hostname>,shard-id=...,tls-port=6379 myself,master - 0 0 0 connected 0-1000
      ```
      Or we can switch the position of two ports, both can be correctly resolved.
      ```
      <node-id> <ip>:<tls_port>@<cport>,<hostname>,shard-id=...,tcp-port=6379 myself,master - 0 0 0 connected 0-1000
      ```
      22a29935
  18. 25 Jun, 2023 2 commits
    • Binbin's avatar
      Move clusterBeforeSleep before blockedBeforeSleep (#12343) · 9600553e
      Binbin authored
      The new blockedBeforeSleep was added in #12337, it breaks the order in 2ecb5edf.
      
      This may be related to #2288, quoted from comment in #2288:
      ```
      Moreover the clusterBeforeSleep() call was misplaced at the end of the chain of the
      beforeSleep() call in redis.c. It should be at the top, before processing un blocking
      clients. This is exactly the reason in the specific instance of the bug as reported,
      why the state was not updated in time before clients served.
      ```
      9600553e
    • Meir Shpilraien (Spielrein)'s avatar
      Fix use after free on blocking RM_Call. (#12342) · 153f8f08
      Meir Shpilraien (Spielrein) authored
      blocking RM_Call was introduced on: #11568, It allows a module to perform
      blocking commands and get the reply asynchronously.If the command gets
      block, a special promise CallReply is returned that allow to set the unblock
      handler. The unblock handler will be called when the command invocation
      finish and it gets, as input, the command real reply.
      
      The issue was that the real CallReply was created using a stack allocated
      RedisModuleCtx which is no longer available after the unblock handler finishes.
      So if the module keeps the CallReply after the unblock handler finished, the
      CallReply holds a pointer to invalid memory and will try to access it when the
      CallReply will be released.
      
      The solution is to create the CallReply with a NULL context to make it totally
      detached and can be freed freely when the module wants.
      
      Test was added to cover this case, running the test with valgrind before the
      fix shows the use after free error. With the fix, there are no valgrind errors.
      
      unrelated: adding a missing `$rd close` in many tests in that file.
      153f8f08
  19. 22 Jun, 2023 2 commits
    • guybe7's avatar
      Modules: Unblock from within a timer coverage (#12337) · 32301999
      guybe7 authored
      Apart from adding the missing coverage, this PR also adds `blockedBeforeSleep`
      that gathers all block-related functions from `beforeSleep`
      
      The order inside `blockedBeforeSleep` is different: now `handleClientsBlockedOnKeys`
      (which may unblock clients) is called before `processUnblockedClients` (which handles
      unblocked clients).
      It makes sense to have this order.
      
      There are no visible effects of the wrong ordering, except some cleanups of the now-unblocked
      client would have  happen in the next `beforeSleep` (will now happen in the current one)
      
      The reason we even got into it is because i triggers an assertion in logresreq.c (breaking
      the assumption that `unblockClient` is called **before** actually flushing the reply to the socket):
      `handleClientsBlockedOnKeys` is called, then it calls `moduleUnblockClientOnKey`, which calls
      `moduleUnblockClient`, which adds the client to `moduleUnblockedClients` back to `beforeSleep`,
      we call `handleClientsWithPendingWritesUsingThreads`, it writes the data of buf to the client, so
      `client->bufpos` became 0
      On the next `beforeSleep`, we call `moduleHandleBlockedClients`, which calls `unblockClient`,
      which calls `reqresAppendResponse`, triggering the assert. (because the `bufpos` is 0) - see https://github.com/redis/redis/pull/12301#discussion_r1226386716
      32301999
    • Gabi Ganam's avatar
      Fix typos in comments (#12338) · 9e5f45f2
      Gabi Ganam authored
      9e5f45f2
  20. 21 Jun, 2023 3 commits
    • Binbin's avatar
      Print strerror when bio initialization fails (#12333) · 47f32bc9
      Binbin authored
      Now we can see something like this:
      ```
      Fatal: Can't initialize Background Jobs. Error message: Cannot allocate memory
      ```
      47f32bc9
    • guybe7's avatar
      Improve moduleBlockClient timeout overflow handling (#12174) · d46ef886
      guybe7 authored
      Continuation of https://github.com/redis/redis/pull/11338
      avoid overflow adding input timeout to "now" in moduleBlockClient.
      d46ef886
    • Madelyn Olson's avatar
      Make nodename test more consistent (#12330) · 73cf0243
      Madelyn Olson authored
      To determine when everything was stable, we couldn't just query the nodename since they aren't API visible by design. Instead, we were using a proxy piece of information which was bumping the epoch and waiting for everyone to observe that. This works for making source Node 0 and Node 1 had pinged, and Node 0 and Node 2 had pinged, but did not guarantee that Node 1 and Node 2 had pinged. Although unlikely, this can cause this failure message. To fix it I hijacked hostnames and used its validation that it has been propagated, since we know that it is stable.
      
      I also noticed while stress testing this sometimes the test took almost 4.5 seconds to finish, which is really close to the current 5 second limit of the log check, so I bumped that up as well just to make it a bit more consistent.
      73cf0243
  21. 20 Jun, 2023 3 commits
    • guybe7's avatar
      Align RM_ReplyWithErrorFormat with RM_ReplyWithError (#12321) · 20fa1560
      guybe7 authored
      Introduced by https://github.com/redis/redis/pull/11923 (Redis 7.2 RC2)
      
      It's very weird and counterintuitive that `RM_ReplyWithError` requires the error-code
      **without** a hyphen while `RM_ReplyWithErrorFormat` requires either the error-code
      **with** a hyphen or no error-code at all
      ```
      RedisModule_ReplyWithError(ctx, "BLA bla bla");
      ```
      vs.
      ```
      RedisModule_ReplyWithErrorFormat(ctx, "-BLA %s", "bla bla");
      ```
      
      This commit aligns RM_ReplyWithErrorFormat to behvae like RM_ReplyWithError.
      it's a breaking changes but it's done before 7.2 goes GA.
      20fa1560
    • Oran Agra's avatar
      Fix broken protocol when PUBLISH emits local push inside MULTI (#12326) · 8ad8f0f9
      Oran Agra authored
      When a connection that's subscribe to a channel emits PUBLISH inside MULTI-EXEC,
      the push notification messes up the EXEC response.
      
      e.g. MULTI, PING, PUSH foo bar, PING, EXEC
      the EXEC's response will contain: PONG, {message foo bar}, 1. and the second PONG
      will be delivered outside the EXEC's response.
      
      Additionally, this PR changes the order of responses in case of a plain PUBLISH (when
      the current client also subscribed to it), by delivering the push after the command's
      response instead of before it.
      This also affects modules calling RM_PublishMessage in a similar way, so that we don't
      run the risk of getting that push mixed together with the module command's response.
      8ad8f0f9
    • judeng's avatar
      use embedded string object and more efficient ll2string for long long value... · 93708c7f
      judeng authored
      
      use embedded string object and more efficient ll2string for long long value convert to string (#12250)
      
      A value of type long long is always less than 21 bytes when convert to a
      string, so always meets the conditions for using embedded string object
      which can always get memory reduction and performance gain (less calls
      to the heap allocator).
      Additionally, for the conversion of longlong type to sds, we also use a faster
      algorithm (the one in util.c instead of the one that used to be in sds.c). 
      
      For the DECR command on 32-bit Redis, we get about a 5.7% performance
      improvement. There will also be some performance gains for some commands
      that heavily use sdscatfmt to convert numbers, such as INFO.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      93708c7f