Home Original page

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

added 2 commits

April 2, 2020 23:40
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.

@nodejs-github-bot 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

@addaleax addaleax added embedding

Issues and PRs related to embedding Node.js in another project.

worker

Issues and PRs related to Worker support.

c++

Issues and PRs that require attention from people who are familiar with C++.

and removed 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

@addaleax addaleax added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Apr 3, 2020

gireeshpunathil

addaleax added a commit that referenced this pull request

Apr 3, 2020 
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>

addaleax added a commit that referenced this pull request

Apr 3, 2020 
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>

BethGriggs pushed a commit that referenced this pull request

Apr 7, 2020 
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>

BethGriggs pushed a commit that referenced this pull request

Apr 7, 2020 
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>

targos pushed a commit that referenced this pull request

Apr 12, 2020 
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 a commit that referenced this pull request

Feb 18, 2021 
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>

addaleax added a commit that referenced this pull request

Feb 18, 2021 
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>

richardlau pushed a commit that referenced this pull request

Feb 22, 2021 
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>

richardlau pushed a commit that referenced this pull request

Feb 22, 2021 
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>