- 03 Oct, 2021 1 commit
-
-
yoav-steinberg authored
Remove hard coded multi-bulk limit (was 1,048,576), new limit is INT_MAX. When client sends an m-bulk that's higher than 1024, we initially only allocate the argv array for 1024 arguments, and gradually grow that allocation as arguments are received.
-
- 26 Sep, 2021 1 commit
-
-
yoav-steinberg authored
Fixing CI test issues introduced in #8687 - valgrind warnings in readQueryFromClient when client was freed by processInputBuffer - adding DEBUG pause-cron for tests not to be time dependent. - skipping a test that depends on socket buffers / events not compatible with TLS - making sure client got subscribed by not using deferring client
-
- 23 Sep, 2021 1 commit
-
-
yoav-steinberg authored
### Description A mechanism for disconnecting clients when the sum of all connected clients is above a configured limit. This prevents eviction or OOM caused by accumulated used memory between all clients. It's a complimentary mechanism to the `client-output-buffer-limit` mechanism which takes into account not only a single client and not only output buffers but rather all memory used by all clients. #### Design The general design is as following: * We track memory usage of each client, taking into account all memory used by the client (query buffer, output buffer, parsed arguments, etc...). This is kept up to date after reading from the socket, after processing commands and after writing to the socket. * Based on the used memory we sort all clients into buckets. Each bucket contains all clients using up up to x2 memory of the clients in the bucket below it. For example up to 1m clients, up to 2m clients, up to 4m clients, ... * Before processing a command and before sleep we check if we're over the configured limit. If we are we start disconnecting clients from larger buckets downwards until we're under the limit. #### Config `maxmemory-clients` max memory all clients are allowed to consume, above this threshold we disconnect clients. This config can either be set to 0 (meaning no limit), a size in bytes (possibly with MB/GB suffix), or as a percentage of `maxmemory` by using the `%` suffix (e.g. setting it to `10%` would mean 10% of `maxmemory`). #### Important code changes * During the development I encountered yet more situations where our io-threads access global vars. And needed to fix them. I also had to handle keeps the clients sorted into the memory buckets (which are global) while their memory usage changes in the io-thread. To achieve this I decided to simplify how we check if we're in an io-thread and make it much more explicit. I removed the `CLIENT_PENDING_READ` flag used for checking if the client is in an io-thread (it wasn't used for anything else) and just used the global `io_threads_op` variable the same way to check during writes. * I optimized the cleanup of the client from the `clients_pending_read` list on client freeing. We now store a pointer in the `client` struct to this list so we don't need to search in it (`pending_read_list_node`). * Added `evicted_clients` stat to `INFO` command. * Added `CLIENT NO-EVICT ON|OFF` sub command to exclude a specific client from the client eviction mechanism. Added corrosponding 'e' flag in the client info string. * Added `multi-mem` field in the client info string to show how much memory is used up by buffered multi commands. * Client `tot-mem` now accounts for buffered multi-commands, pubsub patterns and channels (partially), tracking prefixes (partially). * CLIENT_CLOSE_ASAP flag is now handled in a new `beforeNextClient()` function so clients will be disconnected between processing different clients and not only before sleep. This new function can be used in the future for work we want to do outside the command processing loop but don't want to wait for all clients to be processed before we get to it. Specifically I wanted to handle output-buffer-limit related closing before we process client eviction in case the two race with each other. * Added a `DEBUG CLIENT-EVICTION` command to print out info about the client eviction buckets. * Each client now holds a pointer to the client eviction memory usage bucket it belongs to and listNode to itself in that bucket for quick removal. * Global `io_threads_op` variable now can contain a `IO_THREADS_OP_IDLE` value indicating no io-threading is currently being executed. * In order to track memory used by each clients in real-time we can't rely on updating these stats in `clientsCron()` alone anymore. So now I call `updateClientMemUsage()` (used to be `clientsCronTrackClientsMemUsage()`) after command processing, after writing data to pubsub clients, after writing the output buffer and after reading from the socket (and maybe other places too). The function is written to be fast. * Clients are evicted if needed (with appropriate log line) in `beforeSleep()` and before processing a command (before performing oom-checks and key-eviction). * All clients memory usage buckets are grouped as follows: * All clients using less than 64k. * 64K..128K * 128K..256K * ... * 2G..4G * All clients using 4g and up. * Added client-eviction.tcl with a bunch of tests for the new mechanism. * Extended maxmemory.tcl to test the interaction between maxmemory and maxmemory-clients settings. * Added an option to flag a numeric configuration variable as a "percent", this means that if we encounter a '%' after the number in the config file (or config set command) we consider it as valid. Such a number is store internally as a negative value. This way an integer value can be interpreted as either a percent (negative) or absolute value (positive). This is useful for example if some numeric configuration can optionally be set to a percentage of something else. Co-authored-by:
Oran Agra <oran@redislabs.com>
-
- 19 Sep, 2021 1 commit
-
-
Wen Hui authored
-
- 09 Sep, 2021 1 commit
-
-
yvette903 authored
A write request may be paused unexpectedly because `server.client_pause_end_time` is old. **Recreate this:** redis-cli -p 6379 127.0.0.1:6379> client pause 500000000 write OK 127.0.0.1:6379> client unpause OK 127.0.0.1:6379> client pause 10000 write OK 127.0.0.1:6379> set key value The write request `set key value` is paused util the timeout of 500000000 milliseconds was reached. **Fix:** reset `server.client_pause_end_time` = 0 in `unpauseClients`
-
- 08 Sep, 2021 1 commit
-
-
zhaozhao.zz authored
When a replica paused, it would not apply any commands event the command comes from master, if we feed the non-applied command to replication stream, the replication offset would be wrong, and data would be lost after failover(since replica's `master_repl_offset` grows but command is not applied). To fix it, here are the changes: * Don't update replica's replication offset or propagate commands to sub-replicas when it's paused in `commandProcessed`. * Show `slave_read_repl_offset` in info reply. * Add an assert to make sure master client should never be blocked unless pause or module (some modules may use block way to do background (parallel) processing and forward original block module command to the replica, it's not a good way but it can work, so the assert excludes module now, but someday in future all modules should rewrite block command to propagate like what `BLPOP` does).
-
- 10 Aug, 2021 1 commit
-
-
Meir Shpilraien (Spielrein) authored
Following the comments on #8659, this PR fix some formatting and naming issues.
-
- 09 Aug, 2021 2 commits
-
-
Qu Chen authored
AOF fake client creation (createAOFClient) was doing similar work as createClient, with some minor differences, most of which unintended, this was dangerous and meant that many changes to createClient should have always been reflected to aof.c This cleanup changes createAOFClient to call createClient with NULL, like we do in module.c and elsewhere.
-
Qu Chen authored
Replication client no longer checks incoming command length against the client-query-buffer-limit. This makes the master able to replicate commands longer than replica's configured client-query-buffer-limit
-
- 06 Aug, 2021 1 commit
-
-
yoav-steinberg authored
Also update qbuf tests to verify both idle and peak based resizing logic. And delete unused function: getClientsMaxBuffers
-
- 05 Aug, 2021 1 commit
-
-
yoav-steinberg authored
Reduce dict struct memory overhead on 64bit dict size goes down from jemalloc's 96 byte bin to its 56 byte bin. summary of changes: - Remove `privdata` from callbacks and dict creation. (this affects many files, see "Interface change" below). - Meld `dictht` struct into the `dict` struct to eliminate struct padding. (this affects just dict.c and defrag.c) - Eliminate the `sizemask` field, can be calculated from size when needed. - Convert the `size` field into `size_exp` (exponent), utilizes one byte instead of 8. Interface change: pass dict pointer to dict type call back functions. This is instead of passing the removed privdata field. In the future if we'd like to have private data in the callbacks we can extract it from the dict type. We can extend dictType to include a custom dict struct allocator and use it to allocate more data at the end of the dict struct. This data can then be used to store private data later acccessed by the callbacks.
-
- 14 Jul, 2021 1 commit
-
-
Oran Agra authored
- promote the code in DEBUG PROTOCOL to addReplyBigNum - DEBUG PROTOCOL ATTRIB skips the attribute when client is RESP2 - networking.c addReply for push and attributes generate assertion when called on a RESP2 client, anything else would produce a broken protocol that clients can't handle.
-
- 05 Jul, 2021 2 commits
-
-
Oran Agra authored
This reduces system calls on linux when a new connection is made / accepted. Changes: * Add the SOCK_CLOEXEC option to the accept4() call This ensure that a fork/exec call does not leak a file descriptor. * Move anetCloexec and connNonBlock info anetGenericAccept * Moving connNonBlock from accept handlers to anetGenericAccept Moving connNonBlock from createClient, is safe because createClient is used in the following ways: 1. without a connection (fake client) 2. on an accepted connection (see above) 3. creating the master client by using connConnect (see below) The third case, can either use anetTcpNonBlockConnect, or connTLSConnect which is by default non-blocking. Co-authored-by:
Rajiv Kurian <geetasen@gmail.com> Co-authored-by:
Oran Agra <oran@redislabs.com> Co-authored-by:
Yoav Steinberg <yoav@redislabs.com>
-
zhaozhao.zz authored
1. querybuf_peak has not been updated correctly in readQueryFromClient. 2. qbuf shrinking uses sdsalloc instead of sdsAllocSize see more details in issue #4983
-
- 01 Jul, 2021 1 commit
-
-
Yossi Gottlieb authored
Modules that use background threads with thread safe contexts are likely to use RM_BlockClient() without a timeout function, because they do not set up a timeout. Before this commit, `CLIENT UNBLOCK` would result with a crash as the `NULL` timeout callback is called. Beyond just crashing, this is also logically wrong as it may throw the module into an unexpected client state. This commits makes `CLIENT UNBLOCK` on such clients behave the same as any other client that is not in a blocked state and therefore cannot be unblocked.
-
- 27 Jun, 2021 1 commit
-
-
Yossi Gottlieb authored
-
- 22 Jun, 2021 1 commit
-
-
Yossi Gottlieb authored
* Specifying an empty `bind ""` configuration prevents Redis from listening on any TCP port. Before this commit, such configuration was not accepted. * Using `CONFIG GET bind` will always return an explicit configuration value. Before this commit, if a bind address was not specified the returned value was empty (which was an anomaly). Another behavior change is that modifying the `bind` configuration to a non-default value will NO LONGER DISABLE protected-mode implicitly.
-
- 17 Jun, 2021 1 commit
-
-
sundb authored
Due to the change in #9003, a long-standing bug was raised under `valgrind`. This bug can cause the master-slave sync to take a very long time, causing the `pendingquerybuf.tcl` test to fail. This problem does not only occur in master-slave sync, it is triggered when the big arg is greater than 32k. step: ```sh dd if=/dev/zero of=bigfile bs=1M count=32 ./src/redis-cli -x hset a a < bigfile ``` 1) Make room for querybuf in processMultibulkBuffer, now the alloc of querybuf will be more than 32k. 2) If this happens to trigger the `clientsCronResizeQueryBuffer`, querybuf will be resized to 0. 3) Finally, in readQueryFromClient, we expand the querybuf non-greedily, from 0 to 32k. Old code, make room for querybuf is greedy, so it only needs 11 times to expand to 32M(16k*(2^11)), but now we need 2048(32*1024/16) times to reach it, due to the slow allocation under valgrind that exposed the problem. The fix for the excessive shrinking of the query buf to 0, will be handled in #5013 (that other change on it's own can fix failing test too), but the fix in this PR will also fix the failing test. The fix in this PR will makes the reading in `readQueryFromClient` more aggressive when working on a big arg (so that it is in par with the same code in `processMultibulkBuffer` (i.e. the two calls to `sdsMakeRoomForNonGreedy` should both use the bulk size). In the code before this fix the one in readQueryFromClient always has `readlen = PROTO_IOBUF_LEN`
-
- 16 Jun, 2021 1 commit
-
-
yoav-steinberg authored
Gopher support was added mainly because it was simple (trivial to add). But apparently even something that was trivial at the time, does cause complications down the line when adding more features. We recently ran into a few issues with io-threads conflicting with the gopher support. We had to either complicate the code further in order to solve them, or drop gopher. AFAIK it's completely unused, so we wanna chuck it, rather than keep supporting it.
-
- 15 Jun, 2021 2 commits
-
-
sundb authored
The initialize memory of `querybuf` is `PROTO_IOBUF_LEN(1024*16) * 2` (due to sdsMakeRoomFor being greedy), under `jemalloc`, the allocated memory will be 40k. This will most likely result in the `querybuf` being resized when call `clientsCronResizeQueryBuffer` unless the client requests it fast enough. Note that this bug existed even before #7875, since the condition for resizing includes the sds headers (32k+6). ## Changes 1. Use non-greedy sdsMakeRoomFor when allocating the initial query buffer (of 16k). 1. Also use non-greedy allocation when working with BIG_ARG (we won't use that extra space anyway) 2. in case we did use a greedy allocation, read as much as we can into the buffer we got (including internal frag), to reduce system calls. 3. introduce a dedicated constant for the shrinking (same value as before) 3. Add test for querybuf. 4. improve a maxmemory test by ignoring the effect of replica query buffers (can accumulate many ACKs on slow env) 5. improve a maxmemory by disabling slowlog (it will cause slight memory growth on slow env).
-
yvette903 authored
Fix crash when using io-threads-do-reads and issuing CLIENT PAUSE and CLIENT UNPAUSE. This issue was introduced in redis 6.2 together with the FAILOVER command.
-
- 10 Jun, 2021 1 commit
-
-
Binbin authored
This PR adds a spell checker CI action that will fail future PRs if they introduce typos and spelling mistakes. This spell checker is based on blacklist of common spelling mistakes, so it will not catch everything, but at least it is also unlikely to cause false positives. Besides that, the PR also fixes many spelling mistakes and types, not all are a result of the spell checker we use. Here's a summary of other changes: 1. Scanned the entire source code and fixes all sorts of typos and spelling mistakes (including missing or extra spaces). 2. Outdated function / variable / argument names in comments 3. Fix outdated keyspace masks error log when we check `config.notify-keyspace-events` in loadServerConfigFromString. 4. Trim the white space at the end of line in `module.c`. Check: https://github.com/redis/redis/pull/7751 5. Some outdated https link URLs. 6. Fix some outdated comment. Such as: - In README: about the rdb, we used to said create a `thread`, change to `process` - dbRandomKey function coment (about the dictGetRandomKey, change to dictGetFairRandomKey) - notifyKeyspaceEvent fucntion comment (add type arg) - Some others minor fix in comment (Most of them are incorrectly quoted by variable names) 7. Modified the error log so that users can easily distinguish between TCP and TLS in `changeBindAddr`
-
- 08 Jun, 2021 2 commits
-
-
Wang Yuan authored
When we allocate a client struct with 16k reply buffer, the allocator we may give us 20K, This commit makes use of that extra space. Additionally, it tries to store whatever it can from the reply into the static 'buf' before allocating a new node for the reply list.
-
Yang Bodong authored
It could be useful for debugging to know which client got disconnected.
-
- 19 May, 2021 1 commit
-
-
Madelyn Olson authored
Redact commands that include sensitive data from slowlog and monitor
-
- 04 May, 2021 1 commit
-
-
yoav-steinberg authored
When client breached the output buffer soft limit but then went idle, we didn't disconnect on soft limit timeout, now we do. Note this also resolves some sporadic test failures in due to Linux buffering data which caused tests to fail if during the test we went back under the soft COB limit. Co-authored-by:
Oran Agra <oran@redislabs.com> Co-authored-by:
sundb <sundbcn@gmail.com>
-
- 03 May, 2021 1 commit
-
-
Wen Hui authored
-
- 15 Apr, 2021 2 commits
-
-
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.
-
Bonsai authored
-
- 13 Apr, 2021 1 commit
-
-
Oran Agra authored
server.client_pause_end_time is uninitialized, or actually 0, at startup, which means this method would think the timeout was reached and go look for paused clients. This causes no harm since unpauseClients will not find any paused clients.
-
- 06 Apr, 2021 1 commit
-
-
yjph authored
-
- 31 Mar, 2021 1 commit
-
-
yoav-steinberg authored
this is a followup PR for #8699 instead of copying the deferred reply data to the previous node only if it has room for the entire thing, we can now split the new payload, put part of it into the spare space in the prev node, and the rest may fit into the next node.
-
- 30 Mar, 2021 1 commit
-
-
yoav-steinberg authored
* Avoid checking output limits if buffer didn't grow. * Use previouse node in case it has room in deferred output node.
-
- 29 Mar, 2021 1 commit
-
-
Meir Shpilraien (Spielrein) authored
'processCommandAndResetClient' returns 1 if client is dead. It does it by checking if serve.current_client is NULL. On script timeout, Redis will re-enter 'processCommandAndResetClient' and when finish we will set server.current_client to NULL. This will cause later to falsely return 1 and think that the client that sent the timed-out script is dead (Redis to stop reading from the client buffer).
-
- 25 Mar, 2021 1 commit
-
-
Oran Agra authored
* SLOWLOG didn't record anything for blocked commands because the client was reset and argv was already empty. there was a fix for this issue specifically for modules, now it works for all blocked clients. * The original command argv (before being re-written) was also reset before adding the slowlog on behalf of the blocked command. * Latency monitor is now updated regardless of the slowlog flags of the command or its execution (their purpose is to hide sensitive info from the slowlog, not hide the fact the latency happened). * Latency monitor now uses real_cmd rather than c->cmd (which may be different if the command got re-written, e.g. GEOADD) Changes: * Unify shared code between slowlog insertion in call() and updateStatsOnUnblock(), hopefully prevent future bugs from happening due to the later being overlooked. * Reset CLIENT_PREVENT_LOGGING in resetClient rather than after command processing. * Add a test for SLOWLOG and BLPOP Notes: - real_cmd == c->lastcmd, except inside MULTI and Lua. - blocked commands never happen in these cases (MULTI / Lua) - real_cmd == c->cmd, except for when the command is rewritten (e.g. GEOADD) - blocked commands (currently) are never rewritten - other than the command's CLIENT_PREVENT_LOGGING, and the execution flag CLIENT_PREVENT_LOGGING, other cases that we want to avoid slowlog are on AOF loading (specifically CMD_CALL_SLOWLOG will be off when executed from execCommand that runs from an AOF)
-
- 17 Mar, 2021 1 commit
-
-
Theo Buehler authored
Some operating systems (e.g., NetBSD and OpenBSD) have switched to using a 64-bit integer for time_t on all platforms. This results in currently harmless compiler warnings due to potential truncation. These changes fix these minor portability concerns. * Fix format string for systems with 64 bit time * use llabs to avoid truncation with 64 bit time
-
- 16 Mar, 2021 1 commit
-
-
Madelyn Olson authored
Redact config set requirepass/masterauth/masteruser from slowlog in addition to showing ACL commands without sensitive values.
-
- 21 Feb, 2021 1 commit
-
-
Yossi Gottlieb authored
Originally this was limited to IPv6 address length, but effectively it has been used for host names and now that Sentinel accepts that as well we need to be able to store full hostnames. Fixes #8507
-
- 08 Feb, 2021 1 commit
-
-
Andy Pan authored
-
- 02 Feb, 2021 1 commit
-
-
Huang Zw authored
addReplyLongLongWithPrefix, has a check against negative length, and the code flow removed in this commit bypasses the check. addReplyAggregateLen has an assertion for negative length, but addReplyBulkLen does not, so this commit fixes theoretical case of access violation (probably unreachable though)
-