Disable registry cleanup for count and list operations#517
Open
mccauleyp wants to merge 3 commits intoParallels:masterfrom
Open
Disable registry cleanup for count and list operations#517mccauleyp wants to merge 3 commits intoParallels:masterfrom
mccauleyp wants to merge 3 commits intoParallels:masterfrom
Conversation
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #486
This PR fixes an issue that occurs when the dashboard attempts to obtain job registry counts or list job IDs when it is a) not running in the main thread and b) when those actions trigger a "cleanup" side effect that attempts to invoke job callbacks.
Using the
.countproperty,.get_job_countmethod, or.get_job_idsmethod of theBaseRegistryclass in RQ has a side effect of invoking the.cleanupmethod (see: https://github.com/rq/rq/blob/master/rq/registry.py#L236). For the some registries, namelyStartedJobRegistryandDeferredJobRegistry, this may lead to job callbacks being invoked. This fails if the dashboard is not running in the main thread because it relies on signal handling that can't happen outside of the main thread, leading to this error: #486.The fix here is to simply use the
cleanup=Falseoption to disable the cleanup operation from the dashboard for all registries with the philosophy that the dashboard should generally not be responsible for mutating the state of the job queue as a side effect of displaying it.Why disable cleanup
Rather than disable the cleanup universally as in this PR, we could instead focus on specific registries that are impacted and/or have different behavior for the dashboard depending on whether or not it's running in the main thread. I can revise this PR if that's the consensus, but I don't think the dashboard is the right place to trigger registry cleanup because:
RQ's built-in cleanup mechanism
Workers run maintenance every 10 minutes (https://github.com/rq/rq/blob/master/rq/defaults.py#L66):
Workers check if maintenance should run (https://github.com/rq/rq/blob/master/rq/worker.py#L427-L433):
Workers call clean_registries() with proper exception handlers (https://github.com/rq/rq/blob/master/rq/worker.py#L458-L469):
clean_registries() cleans all registry types (https://github.com/rq/rq/blob/master/rq/registry.py#L551-L571):
A distributed lock prevents redundant cleanup across workers (https://github.com/rq/rq/blob/master/rq/queue.py#L239-L249):
So this PR does not mean that the cleanup won't happen anymore, just that the dashboard won't be triggering it.
Type of change
Please delete options that are not relevant.
Checklist: