Describe the bug

RPOPLPUSH, LMOVE keyspace events are produced in incorrect order. The Redis documentation states: "the lpush event will always be delivered after the rpop event" but at least since Redis 6.2.1 they have been delivered in the opposite order - that is, the push event is delivered first, and then the pop event is delivered. The same behavior is observed in Redis 7.x.

To reproduce

  1. Start a Redis server.
  2. In a redis-cli instance, enable keyspace events (config set notify-keyspace-events KEA).
  3. In another redis-cli, subscribe to keyspace events (redis-cli --csv psubscribe '__key*__:*').
  4. Create a list with at least one member. (lpush key a b c d)
  5. Execute the LMOVE command (lmove key koy left right)

In the redis-cli monitoring keyspace events, the events will be displayed in the wrong order:

"pmessage","__key*__:*","__keyspace@0__:koy","rpush" "pmessage","__key*__:*","__keyevent@0__:rpush","koy" "pmessage","__key*__:*","__keyspace@0__:key","lpop" "pmessage","__key*__:*","__keyevent@0__:lpop","key"

Expected behavior

The events should be delivered in the order described in the documentation.

Additional information

It appears that the keyspace call that handles the pop event (with the comment /* Delete the source list when it is empty */) in t_list.c should be moved up, before the call to lmoveHandlePush().

Comment From: enjoy-binbin

thanks for the report.

for the record: the lpush event will always be delivered after the rpop event, the doc link is in here: https://redis.io/docs/manual/keyspace-notifications/

6.2.0:

[root@binblog redis]# src/redis-cli --csv psubscribe '__key*__:*'
Reading messages... (press Ctrl-C to quit)
"psubscribe","__key*__:*",1
"pmessage","__key*__:*","__keyspace@0__:key","lpush"
"pmessage","__key*__:*","__keyevent@0__:lpush","key"

"pmessage","__key*__:*","__keyspace@0__:koy","rpush"
"pmessage","__key*__:*","__keyevent@0__:rpush","koy"
"pmessage","__key*__:*","__keyspace@0__:key","lpop"
"pmessage","__key*__:*","__keyevent@0__:lpop","key"

unstable (7.0.x):

[root@binblog redis]# src/redis-cli --csv psubscribe '__key*__:*'                                           
Reading messages... (press Ctrl-C to quit)
"psubscribe","__key*__:*",1
"pmessage","__key*__:*","__keyspace@0__:key","lpush"
"pmessage","__key*__:*","__keyevent@0__:lpush","key"

"pmessage","__key*__:*","__keyspace@0__:koy","rpush"
"pmessage","__key*__:*","__keyevent@0__:rpush","koy"
"pmessage","__key*__:*","__keyspace@0__:key","lpop"
"pmessage","__key*__:*","__keyevent@0__:lpop","key"

unstable (after the fix-commit, swap the positions of lmoveHandlePush and listElementsRemoved, i.e. first listElementsRemoved and then lmoveHandlePush, waiting for the consensus of another member. fix the doc or the code...):

[root@binblog redis]# src/redis-cli  --csv psubscribe '__key*__:*'                                           
Reading messages... (press Ctrl-C to quit)
"psubscribe","__key*__:*",1
"pmessage","__key*__:*","__keyspace@0__:key","lpush"
"pmessage","__key*__:*","__keyevent@0__:lpush","key"

"pmessage","__key*__:*","__keyspace@0__:key","lpop"
"pmessage","__key*__:*","__keyevent@0__:lpop","key"
"pmessage","__key*__:*","__keyspace@0__:koy","rpush"
"pmessage","__key*__:*","__keyevent@0__:rpush","koy"

Comment From: oranagra

i run a quick test to see if it was recently broken or not (tested BRPOPLPUSH being blocked). 2.8, 3.0, 3.2, 4.0 are all completely missing the pop notification. 5.0, 6.0, 6.2, 7.0 the pop notification is last (after push).

i then wen't to the documentation blame log to see if maybe it was a careless change in the docs, but that statement appears to be there since the day the keyspace notification doc was added.

[edit] also tested the non-blocking path, in 3.2 and 6.0, the pop notification was last.

Comment From: enjoy-binbin

so looks like just fixing the doc is a safe bet, (and maybe adds some explicit comments / tests to persist this)

Comment From: enjoy-binbin

@oranagra any ideas on how we can move this forward? looks like we just need to fix the doc. (since in the code, the pop notification was always last (after push))

Comment From: oranagra

i think it makes sense to fix the behavior.

the doc is very explicit about what should happen, and it's also consistent with other events (Like MOVE), where we first remove the value from the old place, and only then add it to the new place. i.e. otherwise you have a moment with a state that the value logically in both places.

i suppose that if there's any application that uses these notifications, it's likely that it'll not even be affected by this change. i.e. logically the two actions are still performed, and the application that uses this notification will still handle them correctly (same as it would correctly handle an EVAL script that does POP and then PUSH). in that sense, it might not even be a breaking change, since it produces a scenario that was already possible. @yossigo please share your view on that.

@enjoy-binbin i think you can draft a quick PR, and we can continue the discussion there.