Skip to content

Conversation

@ranjan-purbey
Copy link
Contributor

@ranjan-purbeyranjan-purbey commented Apr 26, 2020

Checklist

In the worker pool example, the kWorkerFreedEvent should be emitted in case of error as well. After adding new worker in the error handler, the pending tasks should be notified of an available worker.

For instance, assume that we create a worker pool with two workers. Then we call runTask thrice such that the first two tasks are still running when it's called for the third time. As a result, the third task will be enqueued and wait for a kWorkerFreedEvent. Now suppose that both of the first two tasks throw an error. In response, our error handler replaces the two existing workers with new ones. But if kWorkerFreedEvent is not emitted after replacing the workers, the third task which was enqueued will never (assuming no more tasks are added after that) know of an available worker and hence will never run.

In the worker pool example, the 'kWorkerFreedEvent' should be emitted in case of error as well. After adding new worker in the error handler, the pending tasks should be notified of an available worker.
@nodejs-github-botnodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations. labels Apr 26, 2020
Copy link
Member

@addaleaxaddaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing this!

emit kWorkerFreedEvent everytime a new worker is created
@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 26, 2020
Copy link
Member

@juanarboljuanarbol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Apr 29, 2020
In the worker pool example, the 'kWorkerFreedEvent' should be emitted in case of error as well. After adding new worker in the error handler, the pending tasks should be notified of an available worker. PR-URL: #33082 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
@addaleax
Copy link
Member

Landed in 393f691, thanks for the PR! 🎉

targos pushed a commit that referenced this pull request May 4, 2020
In the worker pool example, the 'kWorkerFreedEvent' should be emitted in case of error as well. After adding new worker in the error handler, the pending tasks should be notified of an available worker. PR-URL: #33082 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
@targostargos mentioned this pull request May 4, 2020
targos pushed a commit that referenced this pull request May 7, 2020
In the worker pool example, the 'kWorkerFreedEvent' should be emitted in case of error as well. After adding new worker in the error handler, the pending tasks should be notified of an available worker. PR-URL: #33082 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
@targostargos mentioned this pull request May 7, 2020
targos pushed a commit that referenced this pull request May 13, 2020
In the worker pool example, the 'kWorkerFreedEvent' should be emitted in case of error as well. After adding new worker in the error handler, the pending tasks should be notified of an available worker. PR-URL: #33082 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

async_hooksIssues and PRs related to the async hooks subsystem.author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@ranjan-purbey@nodejs-github-bot@addaleax@jasnell@cjihrig@gireeshpunathil@trivikr@juanarbol