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

doc: WorkerPool async_hooks example warns about max listeners #35952

Closed
1 task
loganfsmyth opened this issue Nov 4, 2020 · 0 comments
Closed
1 task

doc: WorkerPool async_hooks example warns about max listeners #35952

loganfsmyth opened this issue Nov 4, 2020 · 0 comments
Labels
doc Issues and PRs related to the documentations.

Comments

@loganfsmyth
Copy link
Contributor

📗 API Reference Docs Problem

  • Version: 15.0.1
  • Platform: All
  • Subsystem: async_hooks

Location

The example WorkerPool using async_hooks

Affected URL(s):

Description

The code I'm explicitly concerned about is

  runTask(task, callback) {
    if (this.freeWorkers.length === 0) {
      // No free threads, wait until a worker thread becomes free.
      this.once(kWorkerFreedEvent, () => this.runTask(task, callback));
      return;
    }

    // ...

Since this code snippet is presented explicitly as how to do something and is very much code that developers could easily end up copy-pasting into their own codebase, I figured it was worth raising as a potential issue.

This code has 2 issues:

  • This quickly triggers a MaxListenersExceededWarning since it adds a listener for every queued task and that number is unbounded
  • This is O(N^2) because when the kWorkerFreedEvent fires, it fires every callback (even though N-1 will just be re-queued), and once listeners have to do a linear search to remove themselves when they fire

Thoughts?

✍️


  • I would like to work on this issue and
    submit a pull request.
@loganfsmyth loganfsmyth added the doc Issues and PRs related to the documentations. label Nov 4, 2020
jasnell added a commit to jasnell/node that referenced this issue Jan 4, 2021
@jasnell jasnell closed this as completed in 3518919 Jan 6, 2021
danielleadams pushed a commit that referenced this issue Jan 12, 2021
Fixes: #35952

PR-URL: #36783
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
targos pushed a commit that referenced this issue May 1, 2021
Fixes: #35952

PR-URL: #36783
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant