Skip to content

Bugfix: Make modules blocked on keys unblock on commands like LPUSH#8356

Merged
oranagra merged 4 commits intoredis:unstablefrom
zuiderkwast:command-unblock-module
Jan 19, 2021
Merged

Bugfix: Make modules blocked on keys unblock on commands like LPUSH#8356
oranagra merged 4 commits intoredis:unstablefrom
zuiderkwast:command-unblock-module

Conversation

@zuiderkwast
Copy link
Contributor

@zuiderkwast zuiderkwast commented Jan 18, 2021

This makes it possible to implement blocking list and zset commands
using the modules API.

It is already supposed to work like this according to the Modules API
reference in the documentation for RedisModule_BlockClientOnKeys():

If you block on a key of a type that has blocking operations
associated, like a list, a sorted set, a stream, and so forth,
the client may be unblocked once the relevant key is targeted
by an operation that normally unblocks the native blocking
operations for that type.

This commit also includes a test case for the reverse: A module
unblocks a client blocked on BLPOP by inserting elements using
RedisModule_ListPush(). This already works, but it was untested.

@zuiderkwast
Make modules blocked on keys unblock on commands like LPUSH
60893a5 
This makes it possible to implement blocking list and zset commands
using the modules API.

It is already supposed to work like this according to the Modules API
reference in the documentation for RedisModule_BlockClientOnKeys():

> If you block on a key of a type that has blocking operations
> associated, like a list, a sorted set, a stream, and so forth,
> the client may be unblocked once the relevant key is targeted
> by an operation that normally unblocks the native blocking
> operations for that type.

This commit also includes a test case for the reverse: A module
unblocks a client blocked on BLPOP by inserting elements using
RedisModule_ListPush(). This already works, but it was untested.
@zuiderkwast zuiderkwast mentioned this pull request Jan 18, 2021
Merged
@oranagra oranagra requested a review from guybe7 January 18, 2021 22:09
@oranagra oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten labels Jan 18, 2021
@oranagra
Copy link
Member

oranagra commented Jan 18, 2021

seems that this is a regression from #7625 (@yangbodong22011 FYI)
P.S. luckily this isn't in 6.0.10. only 6.2 RC.

@oranagra oranagra added state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten and removed state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten labels Jan 18, 2021
yangbodong22011
yangbodong22011 reviewed Jan 19, 2021
View reviewed changes
yangbodong22011
yangbodong22011 reviewed Jan 19, 2021
View reviewed changes
Copy link
Contributor

@yangbodong22011 yangbodong22011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zuiderkwast thanks for your fix, I spent some time to understand your fix, and I have some suggestions for comments to make it easier for others to understand why BLOCKED_MODULE needs to be judged separately.

@oranagra
Copy link
Member

oranagra commented Jan 19, 2021

@zuiderkwast just to be sure. please test it with valgrind:
make distclean; make valgrind then runtest-moduleapi --valgrind

oranagra
oranagra reviewed Jan 19, 2021
View reviewed changes
@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Jan 19, 2021

It passed with valgrind.

@oranagra oranagra merged commit 4985c11 into redis:unstable Jan 19, 2021
@zuiderkwast zuiderkwast deleted the command-unblock-module branch January 19, 2021 13:01
@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Jan 19, 2021

Side note: There's a test case called "Module client blocked on keys does not wake up on wrong type" which you would assume is broken now. It is however still passing because if the key is of wrong type, it goes back to blocking (bpop_reply_callback returns ERR) so it still passes. It's a somewhat confusing test case though, since the module actually wakes up.

@oranagra
Copy link
Member

oranagra commented Jan 19, 2021

@guybe7 please have a look (at both this PR, and the above comment)

@guybe7
Copy link
Collaborator

guybe7 commented Jan 19, 2021

@oranagra PR looks good
@zuiderkwast if that's true now (the module is indeed unblocked) so it was also true before #7625, right?
anyway it's a good point, i suggest to change the blockedonkeys module to count (INCR on some predetermined keyname) how many times get_fsl failed on "wrong type" and change the test to "Module client blocked on keys can wake up on wrong type" so that it'll verify that those LPUSH indeed triggered bpop_reply_callback

@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Jan 19, 2021

it was also true before #7625, right?

Yes, I suppose it was.

change the test to "Module client blocked on keys can wake up on wrong type" so that it'll verify that those LPUSH indeed triggered bpop_reply_callback

Sounds good. But the new test case that I added in this PR also checks that a module can wake up by an LPUSH. Anyway, it's good to test that returning ERR makes it go back to sleep and wakes up again later.

@guybe7
Copy link
Collaborator

guybe7 commented Jan 20, 2021

#8366

@oranagra oranagra mentioned this pull request Jan 31, 2021
Merged
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Mar 2, 2021
@zuiderkwast
Bugfix: Make modules blocked on keys unblock on commands like LPUSH (r…
10253cc 
…edis#8356)

This was a regression from redis#7625 (only in 6.2 RC2).

This makes it possible again to implement blocking list and zset
commands using the modules API.

This commit also includes a test case for the reverse: A module
unblocks a client blocked on BLPOP by inserting elements using
RedisModule_ListPush(). This already works, but it was untested.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

@guybe7 guybe7 Awaiting requested review from guybe7

+1 more reviewer

@yangbodong22011 yangbodong22011 yangbodong22011 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-notes indication that this issue needs to be mentioned in the release notes state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zuiderkwast @oranagra @guybe7 @yangbodong22011