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.

Sign up for GitHub

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

Jump to bottom

worker_threads: fix spawning from preload scripts #37481

Closed
wants to merge 1 commit into from
Closed

worker_threads: fix spawning from preload scripts #37481

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 22, 2021

Fix spawning nested worker threads from preload scripts and
warn about doing so.

Signed-off-by: James M Snell jasnell@gmail.com
Fixes: #36531

@jasnell jasnell requested a review from addaleax February 22, 2021 21:00
@jasnell jasnell added the worker Issues and PRs related to Worker support. label Feb 22, 2021
@jasnell jasnell mentioned this pull request Feb 22, 2021
Closed
addaleax
addaleax reviewed Feb 23, 2021
View reviewed changes
doc/api/worker_threads.md Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Feb 23, 2021

@addaleax ... what would you think of a command line argument that imposed a process-level upper limit on the maximum number of concurrent Workers?

@jasnell
worker_threads: fix spawning from preload scripts
671bbc7 
Fix spawning nested worker threads from preload scripts and
warn about doing so.

Signed-off-by: James M Snell <jasnell@gmail.com>
Fixes: nodejs#36531
@jasnell jasnell force-pushed the fix-preload-workers branch from a2243e1 to 671bbc7 Compare February 23, 2021 14:48
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@addaleax
Copy link
Member

@addaleax ... what would you think of a command line argument that imposed a process-level upper limit on the maximum number of concurrent Workers?

I guess that would be fine – there’s precedent for that kind of thing in the form of ulimit -u (which, at least on Linux, already implements a limit on the number of threads). At the same time, I don’t think this would be something that would be used a ton, either – it’s only useful for catching situations like these, really, but if you know about the command line argument, then you probably also know about how execArgv interacts with Workers in general.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 23, 2021 •
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/36320/ 💚

@jasnell jasnell added the review wanted PRs that need reviews. label Feb 26, 2021
mcollina
mcollina approved these changes Mar 1, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

jasnell added a commit that referenced this pull request Mar 1, 2021
@jasnell
workers: fix spawning from preload scripts
360e8c8 
Fix spawning nested worker threads from preload scripts and
warn about doing so.

Signed-off-by: James M Snell <jasnell@gmail.com>
Fixes: #36531

PR-URL: #37481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Mar 1, 2021

Landed in 360e8c8

@jasnell jasnell closed this Mar 1, 2021
lpinca pushed a commit to lpinca/node that referenced this pull request Mar 2, 2021
@jasnell @lpinca
workers: fix spawning from preload scripts
4281610 
Fix spawning nested worker threads from preload scripts and
warn about doing so.

Signed-off-by: James M Snell <jasnell@gmail.com>
Fixes: nodejs#36531

PR-URL: nodejs#37481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Mar 2, 2021
@jasnell @targos
workers: fix spawning from preload scripts
d93137b 
Fix spawning nested worker threads from preload scripts and
warn about doing so.

Signed-off-by: James M Snell <jasnell@gmail.com>
Fixes: #36531

PR-URL: #37481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos targos mentioned this pull request Mar 2, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@jasnell @targos
workers: fix spawning from preload scripts
78ad8b4 
Fix spawning nested worker threads from preload scripts and
warn about doing so.

Signed-off-by: James M Snell <jasnell@gmail.com>
Fixes: #36531

PR-URL: #37481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
review wanted PRs that need reviews. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Worker thread throws error when started from --require script
4 participants
@jasnell @nodejs-github-bot @addaleax @mcollina