RedisCacheStore#delete_matched blocks and causes timeouts #32610
Labels
Comments
|
Proposed implementation: # The maximum number of entries to receive per SCAN call.
SCAN_BATCH_SIZE = 1000
private_constant :SCAN_BATCH_SIZE
def delete_matched(matcher, options = nil)
instrument :delete_matched, matcher do
unless String === matcher
raise ArgumentError, "Only Redis glob strings are supported: #{matcher.inspect}"
end
redis.with do |c|
start = '0'
pattern = namespace_key(matcher, options)
while true
start, keys = c.scan(start, match: pattern, count: SCAN_BATCH_SIZE)
c.del(*keys) unless keys.empty?
break if start == '0'
end
end
end
end |
glebm
added a commit
to glebm/rails
that referenced
this issue
Apr 17, 2018
Fixes rails#32610 Lua scripts in redis are *blocking*, meaning that no other client can execute any commands while the script is running. See https://redis.io/commands/eval#atomicity-of-scripts. This results in the following exceptions once the number of keys is sufficiently large: BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. This commit replaces the lua-based implementation with one that uses `SCAN` and `DEL` in batches. This doesn't block the server. The primary limitation of `SCAN`, i.e. potential duplicate keys, is of no consequence here, because `DEL` ignores keys that do not exist.
|
SCAN+DEL loop makes sense. Moving to your PR. Thanks @glebm! |
tgxworld
pushed a commit
to tgxworld/rails
that referenced
this issue
Apr 18, 2018
Fixes rails#32610. Closes rails#32614. Lua scripts in redis are *blocking*, meaning that no other client can execute any commands while the script is running. See https://redis.io/commands/eval#atomicity-of-scripts. This results in the following exceptions once the number of keys is sufficiently large: BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. This commit replaces the lua-based implementation with one that uses `SCAN` and `DEL` in batches. This doesn't block the server. The primary limitation of `SCAN`, i.e. potential duplicate keys, is of no consequence here, because `DEL` ignores keys that do not exist.
bogdanvlviv
pushed a commit
to bogdanvlviv/rails
that referenced
this issue
Apr 19, 2018
Fixes rails#32610. Closes rails#32614. Lua scripts in redis are *blocking*, meaning that no other client can execute any commands while the script is running. See https://redis.io/commands/eval#atomicity-of-scripts. This results in the following exceptions once the number of keys is sufficiently large: BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE. This commit replaces the lua-based implementation with one that uses `SCAN` and `DEL` in batches. This doesn't block the server. The primary limitation of `SCAN`, i.e. potential duplicate keys, is of no consequence here, because `DEL` ignores keys that do not exist. cherry-pick ef2af62
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The current implementation of
delete_matchedinRedisCacheStoreis this:rails/activesupport/lib/active_support/cache/redis_cache_store.rb
Lines 232 to 241 in 6378a38
However, lua scripts in redis are blocking, meaning that no other client can execute any commands while the script is running (except
SCRIPT KILLandSHUTDOWN NOSAVE, see https://redis.io/commands/eval#atomicity-of-scripts).This results in the following exceptions once the number of keys is large enough:
Proposed fix is to avoid a script and
SCANthenDELin a loop.SCANis a non-blocking alternative toKEYS. It has some limitations but they do not matter for this use-case./cc @jeremy @fatkodima @georgeclaghorn
The text was updated successfully, but these errors were encountered: