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

Handle peer disconnect in blocking calls #300

Merged
merged 1 commit into from
Nov 6, 2022
Merged

Handle peer disconnect in blocking calls #300

merged 1 commit into from
Nov 6, 2022

Conversation

jgirtakovskis
Copy link
Contributor

@jgirtakovskis jgirtakovskis commented Oct 28, 2022

Problem: Cancelled blocking call to XREADGROUP results in items put on PEL and inaccessible to other subscribers

Scenario:
Connection A writes items [XADD] in one goroutine
Connection B fetches items [XREADGROUP] from second goroutine - blocking
B is in the middle of a blocking call and is cancelled. Client disconnects as expected

Expected: Cancelled blocking call by B does not change the state of the stream and does not put items on PEL
Actual: blocking call proceeds and items are put on PEL

CC: @alicebob

@jgirtakovskis jgirtakovskis changed the title cleanup Handle peer disconnect in blocking calls Oct 28, 2022
@alicebob
Copy link
Owner

Thanks for the PR(s)! I'm getting a race in CI: https://github.com/alicebob/miniredis/actions/runs/3348331876/jobs/5577096047

@jgirtakovskis
Copy link
Contributor Author

jgirtakovskis commented Oct 31, 2022 via email

@alicebob alicebob merged commit e5e64fb into alicebob:master Nov 6, 2022
@alicebob
Copy link
Owner

alicebob commented Nov 6, 2022

Your code look good, the race looks related to a recent change, and I may or may not have fixed that. Thanks for the change! And sorry for the slow replies from my side.

@jgirtakovskis
Copy link
Contributor Author

awesome, thank you!

@jgirtakovskis jgirtakovskis deleted the cleaner-version branch November 7, 2022 17:01
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.

None yet

2 participants