Skip to content
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

[BUG] RPOPLPUSH, LMOVE keyspace events produced in wrong order #11412

Open
ringles opened this issue Oct 20, 2022 · 3 comments
Open

[BUG] RPOPLPUSH, LMOVE keyspace events produced in wrong order #11412

ringles opened this issue Oct 20, 2022 · 3 comments

Comments

@ringles
Copy link

ringles commented Oct 20, 2022

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().

@enjoy-binbin
Copy link
Member

enjoy-binbin commented Oct 21, 2022

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"

@oranagra
Copy link
Member

oranagra commented Oct 23, 2022

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.

@enjoy-binbin
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

3 participants