-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
async.queue pause/resume concurrency problem #512
Conversation
q.process(); | ||
_each(q.tasks, function(task) { | ||
async.setImmediate(q.process); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait -- this calls process()
for each task left in the queue, even if there's 2500+ of them? Surely there's a better way to fix the concurrency. Mabye only call process()
n times where n = concurrency
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right, if the queue contains many tasks the cycle can be heavy.
Could be so:
for (var i = 0; i < q.tasks.length && i < q.concurrency; i++) {
async.setImmediate(q.process);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work a lot better.
Can you please add a failing test for this. I think it's a legitimate issue, I just want the patch to be accompanied by a test case. |
Looks like this bug is (still?) appearing in NPM's package, v0.9.0. Any updates since then? |
I just added a couple tests for this and it seems to work fine. Am I testing what is causing the issue properly? |
In the async.queue I found an issue related to restart after a pause (resume) and I try to fix.
It's not simple to explain, please follow these steps:
Example of problem: