Revert "embedding: make Stop() stop Workers" by addaleax · Pull Request #32623 · nodejs/node · GitHub

Revert "embedding: make Stop() stop Workers"

This reverts commit 037ac99.

As flaky CI failures have revealed, this feature was implemented
incorrectly. stop_sub_worker_contexts() needs to be called on the
thread on which the Environment is currently running, it’s not
thread-safe. The current API requires Stop() to be thread-safe,
though.

We could add a new API for this, but unless there’s demand, that’s
probably not necessary as FreeEnvironment() will also stop Workers,
which is commonly the next action on an Environment instance after
having Stop() called on it.

Refs: #32531

src,test: add regression test for nested Worker termination

This adds a regression test for terminating a Worker inside which
another Worker is running.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

This reverts commit 037ac99. As flaky CI failures have revealed, this feature was implemented incorrectly. `stop_sub_worker_contexts()` needs to be called on the thread on which the `Environment` is currently running, it’s not thread-safe. The current API requires `Stop()` to be thread-safe, though. We could add a new API for this, but unless there’s demand, that’s probably not necessary as `FreeEnvironment()` will also stop Workers, which is commonly the next action on an `Environment` instance after having `Stop()` called on it. Refs: nodejs#32531

This adds a regression test for terminating a Worker inside which another Worker is running.

Fast-track?

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

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

This reverts commit 037ac99. As flaky CI failures have revealed, this feature was implemented incorrectly. `stop_sub_worker_contexts()` needs to be called on the thread on which the `Environment` is currently running, it’s not thread-safe. The current API requires `Stop()` to be thread-safe, though. We could add a new API for this, but unless there’s demand, that’s probably not necessary as `FreeEnvironment()` will also stop Workers, which is commonly the next action on an `Environment` instance after having `Stop()` called on it. Refs: #32531 PR-URL: #32623 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

This adds a regression test for terminating a Worker inside which another Worker is running. PR-URL: #32623 Refs: #32531 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Landed in 7947811...e629366

This reverts commit 037ac99. As flaky CI failures have revealed, this feature was implemented incorrectly. `stop_sub_worker_contexts()` needs to be called on the thread on which the `Environment` is currently running, it’s not thread-safe. The current API requires `Stop()` to be thread-safe, though. We could add a new API for this, but unless there’s demand, that’s probably not necessary as `FreeEnvironment()` will also stop Workers, which is commonly the next action on an `Environment` instance after having `Stop()` called on it. Refs: #32531 PR-URL: #32623 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

This adds a regression test for terminating a Worker inside which another Worker is running. PR-URL: #32623 Refs: #32531 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

This reverts commit 037ac99. As flaky CI failures have revealed, this feature was implemented incorrectly. `stop_sub_worker_contexts()` needs to be called on the thread on which the `Environment` is currently running, it’s not thread-safe. The current API requires `Stop()` to be thread-safe, though. We could add a new API for this, but unless there’s demand, that’s probably not necessary as `FreeEnvironment()` will also stop Workers, which is commonly the next action on an `Environment` instance after having `Stop()` called on it. Refs: #32531 PR-URL: #32623 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

This adds a regression test for terminating a Worker inside which another Worker is running. PR-URL: #32623 Refs: #32531 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

I added this to v12.x-staging because people were reporting this bug occurring in v12.20.1, and it's a change that should definitely land everywhere where the original bug was also introduced. /cc @nodejs/lts

This reverts commit 037ac99. As flaky CI failures have revealed, this feature was implemented incorrectly. `stop_sub_worker_contexts()` needs to be called on the thread on which the `Environment` is currently running, it’s not thread-safe. The current API requires `Stop()` to be thread-safe, though. We could add a new API for this, but unless there’s demand, that’s probably not necessary as `FreeEnvironment()` will also stop Workers, which is commonly the next action on an `Environment` instance after having `Stop()` called on it. Refs: #32531 PR-URL: #32623 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

This adds a regression test for terminating a Worker inside which another Worker is running. PR-URL: #32623 Refs: #32531 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

addaleax added 2 commits April 2, 2020 23:40

src,test: add regression test for nested Worker termination

80d4ba8

This adds a regression test for terminating a Worker inside which another Worker is running.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 2, 2020

This was referenced Apr 2, 2020

embedding: provide hook for custom process.exit() behaviour #32531

Closed

workers: implement --max-worker-threads command line option #32606

Closed

addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 2, 2020

jasnell approved these changes Apr 2, 2020

View reviewed changes

cjihrig approved these changes Apr 3, 2020

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 3, 2020

gireeshpunathil approved these changes Apr 3, 2020

View reviewed changes

addaleax closed this Apr 3, 2020

addaleax deleted the revert-stop-workers branch April 3, 2020 02:33

targos added dont-land-on-v10.x labels Apr 11, 2020

targos mentioned this pull request Apr 13, 2020

v13.13.0 release proposal #32813

Merged

BethGriggs mentioned this pull request Apr 14, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

joyeecheung mentioned this pull request Jun 8, 2020

src: retrieve binding data from the context #33139

Closed

3 tasks

addaleax removed the dont-land-on-v12.x label Feb 18, 2021

nicolo-ribaudo mentioned this pull request Feb 18, 2021

Run Babel asynchronously in @babel/register babel/babel#12814

Closed

richardlau mentioned this pull request Mar 18, 2021

v12.22.0 proposal #37797

Merged

Revert "embedding: make Stop() stop Workers" by addaleax · Pull Request #32623 · nodejs/node · GitHub

Uh oh!

Uh oh!

Revert "embedding: make Stop() stop Workers" - #32623

Revert "embedding: make Stop() stop Workers"#32623
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:revert-stop-workers

addaleax commented Apr 2, 2020

Uh oh!

addaleax commented Apr 2, 2020

Uh oh!

nodejs-github-bot commented Apr 2, 2020

Uh oh!

nodejs-github-bot commented Apr 3, 2020

Uh oh!

addaleax commented Apr 3, 2020

Uh oh!

addaleax commented Feb 18, 2021 •
edited
Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Revert "embedding: make Stop() stop Workers" by addaleax · Pull Request #32623 · nodejs/node · GitHub

Uh oh!

Uh oh!

Revert "embedding: make Stop() stop Workers" - #32623

Revert "embedding: make Stop() stop Workers"#32623addaleax wants to merge 2 commits intonodejs:masternodejs/node:masterfrom addaleax:revert-stop-workersaddaleax/node:revert-stop-workersCopy head branch name to clipboard

Conversation

addaleax commented Apr 2, 2020

Revert "embedding: make Stop() stop Workers"

src,test: add regression test for nested Worker termination

Checklist

Uh oh!

addaleax commented Apr 2, 2020

Uh oh!

nodejs-github-bot commented Apr 2, 2020

Uh oh!

nodejs-github-bot commented Apr 3, 2020

Uh oh!

addaleax commented Apr 3, 2020

Uh oh!

addaleax commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Revert "embedding: make Stop() stop Workers"#32623
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:revert-stop-workers

addaleax commented Feb 18, 2021 •
edited
Loading