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

Increase blocking timeout to harden the blocking pop test case #1128

Merged
merged 1 commit into from
Nov 26, 2022
Merged

Increase blocking timeout to harden the blocking pop test case #1128

merged 1 commit into from
Nov 26, 2022

Conversation

git-hulk
Copy link
Member

@git-hulk git-hulk commented Nov 19, 2022

close #1126

Currently, the blocking pop with the empty argument may fail if took a long time(more than 1s) to execute the RPUSH command. The leading cause is the previous BRPOP command would get the empty bulk string($-1\r\n) instead of the element array when it's timed out.

Currently, the blocking pop with the empty argument may fail if
took long time(more than 1s) to execute the RPUSH command, then
the previous brpop command would get the empty bulk string(`$-1\r\n`)
instead of the element array.
@git-hulk
Copy link
Member Author

I think 4 seconds is long enough since the default read/write timeout of the Redis client is 3 seconds.

@git-hulk
Copy link
Member Author

git-hulk commented Nov 19, 2022

https://github.com/apache/incubator-kvrocks/actions/runs/3502202012/jobs/5866498105#step:12:209

Looks like the bug was caused by the blocking client wasn't wake up after pushing the new element and then response nil bulk string after the timeout was reached.

Update: I found the root cause should be a corner case that the blocking pop may be blocking until timeout even that has elements in the list.

The condition like below:

Time       Thread A                       Thread B

T0:     received BRPOP                  received RPUSH

T1:     try to pop but got empty 

T2:                                     push the element

T3:                                     try to notify blocking clients but no one is blocking

T4:     add to blocking clients 

I may some time to think about how to fix this issue. To see folks have any comments or thoughts on this? @tisonkun @ShooterIT @torwig @torwig

@PragmaTwice
Copy link
Member

PragmaTwice commented Nov 19, 2022

How about introduce some mutexes? like map(key -> mutex). (After a cursory glance)

@tisonkun
Copy link
Member

Merging as a net win...

@tisonkun tisonkun merged commit 5467b02 into apache:unstable Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unstable Go case list_test
3 participants