Forbid non-idempotent block in Redis pipelined/multi
Description of the proposal
This rubocop flags potential non-idempotent operations inside a redis.multi
or redis.pipelined
block.
The way it's implemented is by only allowing send nodes with multi
as the receiver and nothing else, ie:
# bad
Gitlab::Redis::SharedState.with do |redis|
redis.multi do |multi|
multi.set(...)
new = multi.incr(...) # variable assignment here is not allowed
end
end
new.value
# good
Gitlab::Redis::SharedState.with do |redis|
redis.multi do |multi|
multi.set(...) # any method of multi is allowed
multi.incr(...)
end
end
Why?
This thread explains the situation perfectly.
Redis stores (ie Gitlab::Redis::SharedState
) can be wrapped in a Multistore migration helper which means commands will be double read/written to the new Redis store.
When using multi/pipelined command, variable assignment can result in a wrong value due to:
- Result from a Redis command in multi/pipelined block is first stored in a future https://github.com/redis/redis-rb/tree/v4.8.1#futures
- The variable then holds the value from the second Redis command.
- This is because of the way Multistore implements
write_both
which writes to the default store first followed by the non-default store.
- This is because of the way Multistore implements
In the S2 incident, CachedQuota.track_consumption
, the new_balance
is first assigned from the SharedState's (original store) result, followed by reassigning to the ClusterSharedState's (new store we're migrating to) result. This makes new_balance
store hold the wrong value from ClusterSharedState. The fix is introduced in !137734 (diffs).
In a nutshell, we shouldn't do any non-idempotent operation in the Redis pipelined
/ multi
block.
Limitations
- This cop starts checking from blocks and filters only
multi
andpipelined
command. At the time of writing,redis
connection is the only receiver of.multi
and.pipelined
nodes. - This cop will catch more false negatives as we only allow a pattern like:
redis.multi do |m| m.call(...) m.call(...) end
- This means we still need to resort to our judgment on whether the violating blocks are idempotent even though it's flagged by this cop.
Check-list
-
Make sure this MR enables a static analysis check rule for new usage but ignores current offenses. -
Mention this proposal in the relevant Slack channels (e.g. #development
,#backend
,#frontend
). -
If there is a choice to make between two potential styles, set up an emoji vote in the MR: - CHOICE_A:
🅰 - CHOICE_B:
🅱 - Vote for both choices, so they are visible to others.
- CHOICE_A:
-
The MR doesn't have significant objections, and is getting a majority of 👍 vs👎 (remember that we don't need to reach a consensus). -
(If applicable) One style is getting a majority of vote (compared to the other choice). -
(If applicable) Update the MR with the chosen style. -
Create a follow-up issue to fix the current offenses as a separate iteration: ISSUE_LINK -
Follow the review process as usual. -
Once approved and merged by a maintainer, mention it again: -
In the relevant Slack channels (e.g. #development
,#backend
,#frontend
). -
(Optional depending on the impact of the change) In the Engineering Week in Review.
-