-
Notifications
You must be signed in to change notification settings - Fork 24.4k
Only mark the client reprocessing flag when unblocked on keys #14165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When we refactored the blocking framework we introduced the client
reprocessing infrastructure. In cases the client was blocked on keys, it
will attempt to reprocess the command. One challenge was to keep track
of the command timeout, since we are reprocessing and do not want to
re-register the client with a fresh timeout each time. The solution was
to consider the client reprocessing flag when the client is
blockedOnKeys:
```
if (!c->flag.reprocessing_command) {
/* If the client is re-processing the command, we do not set the timeout
* because we need to retain the client's original timeout. */
c->bstate->timeout = timeout;
}
```
However, this introduced a new issue. There are cases where the client
will consecutive blocking of different types for example:
```
CLIENT PAUSE 10000 ALL
BZPOPMAX zset 1
```
would have the client blocked on the zset endlessly if nothing will be
written to it.
**Credits to @uriyage for locating this with his fuzzer testing**
The suggested solution is to only flag the client when it is
specifically unblocked on keys.
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
ShooterIT
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but feel the title is too internal, how about Fix missing timeout update when reprocessing command with different block type, you may have better one
|
I have a question, does it have a problem? if we don't have any operation, |
yes. |
|
New Issues (5)Checkmarx found the following issues in this Pull Request
Fixed Issues (7)Great job! The following issues were fixed in this Pull Request
|
This is the General Availability release of Redis Open Source 8.2. ### Major changes compared to 8.0 - Streams - new commands: `XDELEX` and `XACKDEL`; extension to `XADD` and `XTRIM` - Bitmap - `BITOP`: new operators: `DIFF`, `DIFF1`, `ANDOR`, and `ONE` - Query Engine - new SVS-VAMANA vector index type which supports vector compression - More than 15 performance and resource utilization improvements - New metrics: per-slot usage metrics, key size distributions for basic data types, and more ### Binary distributions - Alpine and Debian Docker images - https://hub.docker.com/_/redis - Install using snap - see https://github.com/redis/redis-snap - Install using brew - see https://github.com/redis/homebrew-redis - Install using RPM - see https://github.com/redis/redis-rpm - Install using Debian APT - see https://github.com/redis/redis-debian ### Operating systems we test Redis 8.2 on - Ubuntu 22.04 (Jammy Jellyfish), 24.04 (Noble Numbat) - Rocky Linux 8.10, 9.5 - AlmaLinux 8.10, 9.5 - Debian 12 (Bookworm) - macOS 13 (Ventura), 14 (Sonoma), 15 (Sequoia) ### Security fixes (compared to 8.2-RC1) - (CVE-2025-32023) Fix out-of-bounds write in `HyperLogLog` commands - (CVE-2025-48367) Retry accepting other connections even if the accepted connection reports an error ### New Features (compared to 8.2-RC1) - #14141 Keyspace notifications - new event types: - `OVERWRITTEN` - the value of a key is completely overwritten - `TYPE_CHANGED` - key type change ### Bug fixes (compared to 8.2-RC1) - #14162 Crash when using evport with I/O threads - #14163 `EVAL` crash when error table is empty - #14144 Vector sets - RDB format is not compatible with big endian machines - #14165 Endless client blocking for blocking commands - #14164 Prevent `CLIENT UNBLOCK` from unblocking `CLIENT PAUSE` - #14216 TTL was not removed by the `SET` command - #14224 `HINCRBYFLOAT` removes field expiration on replica ### Performance and resource utilization improvements (compared to 8.2-RC1) - #14200 Store iterators on stack instead of on heap - #14144 Vector set - improve RDB loading / RESTORE speed by storing the worst link info - #Q6430 More compression variants for the SVS-VAMANA vector index - #Q6535 `SHARD_K_RATIO` parameter - favor network latency over accuracy for KNN vector query in a Redis cluster (unstable feature) (MOD-10359) ### Modules API - #14051 `RedisModule_Get*`, `RedisModule_Set*` - allow modules to access Redis configurations - #14114 `RM_UnsubscribeFromKeyspaceEvents` - unregister a module from specific keyspace notifications
…14165) This PR is based on valkey-io/valkey#2109 When we refactored the blocking framework we introduced the client reprocessing infrastructure. In cases the client was blocked on keys, it will attempt to reprocess the command. One challenge was to keep track of the command timeout, since we are reprocessing and do not want to re-register the client with a fresh timeout each time. The solution was to consider the client reprocessing flag when the client is blockedOnKeys: ```c if (!(c->flags & CLIENT_REPROCESSING_COMMAND)) { /* If the client is re-processing the command, we do not set the timeout * because we need to retain the client's original timeout. */ c->bstate.timeout = timeout; } ``` However, this introduced a new issue. There are cases where the client will consecutive blocking of different types for example: ``` CLIENT PAUSE 10000 ALL BZPOPMAX zset 1 ``` would have the client blocked on the zset endlessly if nothing will be written to it. **Credits to @uriyage for locating this with his fuzzer testing** The suggested solution is to only flag the client when it is specifically unblocked on keys. Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Binbin <binloveplay1314@qq.com>
…14165) This PR is based on valkey-io/valkey#2109 When we refactored the blocking framework we introduced the client reprocessing infrastructure. In cases the client was blocked on keys, it will attempt to reprocess the command. One challenge was to keep track of the command timeout, since we are reprocessing and do not want to re-register the client with a fresh timeout each time. The solution was to consider the client reprocessing flag when the client is blockedOnKeys: ```c if (!(c->flags & CLIENT_REPROCESSING_COMMAND)) { /* If the client is re-processing the command, we do not set the timeout * because we need to retain the client's original timeout. */ c->bstate.timeout = timeout; } ``` However, this introduced a new issue. There are cases where the client will consecutive blocking of different types for example: ``` CLIENT PAUSE 10000 ALL BZPOPMAX zset 1 ``` would have the client blocked on the zset endlessly if nothing will be written to it. **Credits to @uriyage for locating this with his fuzzer testing** The suggested solution is to only flag the client when it is specifically unblocked on keys. Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Binbin <binloveplay1314@qq.com>



This PR is based on valkey-io/valkey#2109
When we refactored the blocking framework we introduced the client reprocessing infrastructure. In cases the client was blocked on keys, it will attempt to reprocess the command. One challenge was to keep track of the command timeout, since we are reprocessing and do not want to re-register the client with a fresh timeout each time. The solution was to consider the client reprocessing flag when the client is blockedOnKeys:
However, this introduced a new issue. There are cases where the client will consecutive blocking of different types for example:
would have the client blocked on the zset endlessly if nothing will be written to it.
Credits to @uriyage for locating this with his fuzzer testing
The suggested solution is to only flag the client when it is specifically unblocked on keys.
Signed-off-by: Ran Shidlansik ranshid@amazon.com
Co-authored-by: Ran Shidlansik ranshid@amazon.com
Co-authored-by: Binbin binloveplay1314@qq.com