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 does not resolve/reject all promises on concurrent execution #64

Closed
domoran opened this issue Nov 10, 2019 · 1 comment · Fixed by #65
Closed

Queue does not resolve/reject all promises on concurrent execution #64

domoran opened this issue Nov 10, 2019 · 1 comment · Fixed by #65
Labels

Comments

@domoran
Copy link

domoran commented Nov 10, 2019

For a simple port scanning script I queued executions of port scans expecting each of the scans to either resolve (port opened) or reject (port closed).

Another problem I found is that the queue was not really executing concurrently. After analyzing the reason for this I found the problem on the dequeue() method, which is broken in multiple ways:

return Promise.all(promises) .then(values => { for (let output of values) this.emit("resolve", output); return values;# }) .catch(error => { this.emit("reject", error); return error; }) .then(output => { this.finalize(); return output; });

Promise.all will reject when the first promise rejects! This has two bad effects:

  1. This leads to finalize only being called once even if multiple promises should have been dequeued, leading the currentlyHandled counter to stay one below max, not allowing concurrent execution anymore but just continue with serial execution

  2. No "reject", "resolve" events will be emitted for the executed tasks, that come after the resolve.

I will submit a pull request for a fix.

domoran pushed a commit to domoran/queue-promise that referenced this issue Nov 10, 2019
  - make sure concurrently handled promises will resolve
  - make sure that concurrent execution really executes consurrently

- See: Bartozzz#64
@Bartozzz
Copy link
Owner

Bartozzz commented Nov 11, 2019

@domoran thanks for your PR! I updated a few things and it should be working on queue-promise@1.3.2. Here's the full changelog: https://github.com/Bartozzz/queue-promise/releases/tag/v1.3.2.

GitHub
A simple, dependency-free library for concurrent promise-based queues. Comes with with concurrency and timeout control. - Bartozzz/queue-promise

@Bartozzz Bartozzz added the bug label Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants