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

Reaper manager registration is subject to race conditions #801

Closed
leandrogoe opened this issue Aug 4, 2023 · 1 comment · Fixed by #820
Closed

Reaper manager registration is subject to race conditions #801

leandrogoe opened this issue Aug 4, 2023 · 1 comment · Fixed by #820

Comments

@leandrogoe
Copy link

leandrogoe commented Aug 4, 2023

Describe the bug
The reaper manager registration is subject to race conditions, which means multiple reapers can potentially start at the same time causing extra load in redis. Bug found on version v7.1.30.

Expected behavior
There should be only one reaper manager, even when running multiple Sidekiq processes.

Current behavior
The bug can be easily replicated using the following snippet:

> 1000.times { |index| Thread.new { sleep(rand()); SidekiqUniqueJobs::Orphans::Manager.start }  } 
2023-08-04T12:22:56.128Z 86290 TID-oxfywtnt6 uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:56.129Z 86290 TID-oxfywuq7a uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:56.129Z 86290 TID-oxfywt5fm uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:56.129Z 86290 TID-oxfywqgxi uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:56.129Z 86290 TID-oxfywulj6 uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:56.129Z 86290 TID-oxfywt64y uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:57.141Z 86290 TID-oxfwnhp82 uniquejobs=orphan-reaper INFO: Nothing to delete; exiting.
2023-08-04T12:22:57.144Z 86290 TID-oxfwnhp82 uniquejobs=orphan-reaper INFO: Nothing to delete; exiting.
2023-08-04T12:22:56.612Z 86290 TID-oxfywtjim uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:56.613Z 86290 TID-oxfywqen6 uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:56.613Z 86290 TID-oxfywt5sq uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:56.613Z 86290 TID-oxfywtoq6 uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:56.613Z 86290 TID-oxfywqhpu uniquejobs=orphan-reaper INFO: Starting Reaper
2023-08-04T12:22:56.613Z 86290 TID-oxfywt28a uniquejobs=orphan-reaper INFO: Starting Reaper
1000

As you can see multiple managers were registered, but there should only be one.

On our deployment, we have frequent Sidekiq restarts, which exacerbates this problem, and we ended up running over 20+ reaper managers at the same time, what cripples Redis performance.

@leandrogoe leandrogoe changed the title Reaper manager subejct to race conditions Reaper manager registration is subejct to race conditions Aug 4, 2023
@leandrogoe leandrogoe changed the title Reaper manager registration is subejct to race conditions Reaper manager registration is subject to race conditions Aug 22, 2023
@mhenrixon
Copy link
Owner

I am hoping #820 will help with this. I don't think that more than one process can be registered. It was likely just that the logging happened despite the registration not going through.

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 a pull request may close this issue.

2 participants