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

Queue#run fires a processStalledJobs loop when concurrency is 0 #327

Closed
xdc0 opened this issue Aug 1, 2016 · 5 comments
Closed

Queue#run fires a processStalledJobs loop when concurrency is 0 #327

xdc0 opened this issue Aug 1, 2016 · 5 comments

Comments

@xdc0
Copy link
Contributor

xdc0 commented Aug 1, 2016

I noticed this with my use case, where the concurrency value is set through an environment variable, this way you can have a service running on many instances with configurable concurrency values on each of them. A simple way to prevent a worker from processing jobs is by setting concurrency to 0 when it's needed.

However, setting the concurrency value to 0 by calling queue.process(0, myJobHandler) has the side effect of always running a processStalledJobs loop, this means that a worker that is not intended to run any jobs will run jobs through the processStalledJobs loop, as a side note, processStalledJobs currently has a flaw where it also processes fresh jobs, not only stalled ones.

@xdc0
Copy link
Contributor Author

xdc0 commented Aug 1, 2016

While I can argue that calling Queue#process with 0 concurrency is an anti-pattern, it should have tighter constraints, either explicitly throw when setting concurrency as a non positive integer value, or return early when concurrency <= 0 so it doesn't do any process at all.

@manast
Copy link
Member

manast commented Sep 1, 2016

can you provide more information regarding the issue in processStalledJobswhere it also processes fresh jobs?

@bradvogel
Copy link
Contributor

bradvogel commented Sep 1, 2016

It's the issue described here: #257. So processStalledJobs is actually processing quite a few fresh jobs.

@bradvogel
Copy link
Contributor

Fixed in #359

@doublerebel
Copy link
Contributor

Looks like #359 was merged so this can be closed?

@manast manast closed this as completed Nov 22, 2016
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

No branches or pull requests

4 participants