process,worker: ensure code after exit() effectless by ywave620 · Pull Request #45620 · nodejs/node · GitHub

Cope with the delay(to the next function call) of v8::Isolate::TerminateExecution()

I believe the original author forgot to add this import. And this is why the strictEqual below paased, because adding to the workerData raises an reference exception.

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

Commit Queue failed

- Loading data for nodejs/node/pull/45620
✔  Done loading data for nodejs/node/pull/45620
----------------------------------- PR info ------------------------------------
Title      process,worker: ensure code followed by exit() effectless (#45620)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     ywave620:ensure-exit -> nodejs:main
Labels     process, author ready, needs-ci
Commits    1
 - process,worker: ensure code followed by exit() effectless
Committers 1
 - ywave620 
PR-URL: https://github.com/nodejs/node/pull/45620
Reviewed-By: Anna Henningsen 
Reviewed-By: Yagiz Nizipli 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45620
Reviewed-By: Anna Henningsen 
Reviewed-By: Yagiz Nizipli 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 25 Nov 2022 07:56:35 GMT
   ✔  Approvals: 2
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/45620#pullrequestreview-1194537461
   ✔  - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/45620#pullrequestreview-1194617601
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2022-11-26T10:52:29Z: https://ci.nodejs.org/job/node-test-pull-request/48195/
- Querying data for job/node-test-pull-request/48195/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/3557316810

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

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

Cope with the delay(to the next function call) of v8::Isolate::TerminateExecution()

spurious failure

[ RUN      ] InspectorSocketTest.HostIPv6NonRoutable
../test/cctest/test_inspector_socket.cc:323: Failure
Value of: (!expectation.read_expected)
  Actual: true
Expected: false

fix typo

ignore exception that indicaties a termination in napi call

Could you take a look at the CI failure?

https://ci.nodejs.org/job/node-test-commit-arm-debug/5274/nodes=ubuntu2004_debug-arm64/testReport/junit/(root)/test/node_api_test_worker_terminate_test/

Fix. PTAL, especially change in src

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

Note that if this->isolate->IsExecutionTerminating() is true, we can not call can_call_into_js(), because that will extract the node::Environment by calling v8 API on this->isolate , which will again hit the assertion:

# Fatal error in ../deps/v8/src/api/api.cc, line 8635
# Debug check failed: !i_isolate->is_execution_terminating().

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

Landed in 8438f3b

Cope with the delay(to the next function call) of v8::Isolate::TerminateExecution() PR-URL: #45620 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Nov 25, 2022

ywave620 commented Nov 25, 2022

View reviewed changes

addaleax approved these changes Nov 25, 2022

View reviewed changes

anonrig approved these changes Nov 25, 2022

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 26, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 26, 2022

github-actions Bot mentioned this pull request Nov 27, 2022

CI Reliability 2022-11-27 nodejs/reliability#441

Open

21 tasks

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2022

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 27, 2022

jasnell approved these changes Nov 27, 2022

View reviewed changes

cjihrig approved these changes Nov 27, 2022

View reviewed changes

github-actions Bot mentioned this pull request Nov 28, 2022

CI Reliability 2022-11-28 nodejs/reliability#442

Open

20 tasks

process,worker: ensure code after exit() effectless

de62467

Cope with the delay(to the next function call) of v8::Isolate::TerminateExecution()

ywave620 force-pushed the ensure-exit branch from c7f350d to 5bf6a3c Compare November 28, 2022 09:16

ywave620 changed the title process,worker: ensure code followed by exit() effectless process,worker: ensure code after exit() effectless Nov 28, 2022

JungMinu approved these changes Nov 28, 2022

View reviewed changes

This was referenced Nov 29, 2022

CI Reliability 2022-11-29 nodejs/reliability#443

Open

CI Reliability 2022-11-30 nodejs/reliability#444

Open

CI Reliability 2022-12-01 nodejs/reliability#445

Open

CI Reliability 2022-12-02 nodejs/reliability#446

Open

process,worker: ensure code after exit() effectless

37cccd0

fix typo

ywave620 force-pushed the ensure-exit branch from 5bf6a3c to 37cccd0 Compare December 2, 2022 01:56

github-actions Bot mentioned this pull request Dec 10, 2022

CI Reliability 2022-12-10 nodejs/reliability#453

Open

9 tasks

process,worker: ensure code after exit() effectless

0c91220

ignore exception that indicaties a termination in napi call

ywave620 force-pushed the ensure-exit branch from f3b73e2 to 0c91220 Compare December 10, 2022 11:31

daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2022

This was referenced Dec 11, 2022

CI Reliability 2022-12-11 nodejs/reliability#454

Open

CI Reliability 2022-12-12 nodejs/reliability#455

Open

ywave620 commented Dec 12, 2022

View reviewed changes

daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 12, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 12, 2022

github-actions Bot mentioned this pull request Dec 13, 2022

CI Reliability 2022-12-13 nodejs/reliability#456

Open

13 tasks

ywave620 requested review from addaleax and aduh95 and removed request for addaleax December 25, 2022 03:05

aduh95 approved these changes Dec 25, 2022

View reviewed changes

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 25, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2022

nodejs-github-bot merged commit 8438f3b into nodejs:main Dec 25, 2022

RafaelGSS mentioned this pull request Jan 2, 2023

v19.4.0 proposal #46061

Merged

juanarbol mentioned this pull request Jan 28, 2023

V18.14.0 proposal #46396

Merged

process,worker: ensure code after exit() effectless by ywave620 · Pull Request #45620 · nodejs/node · GitHub

Uh oh!

Uh oh!

process,worker: ensure code after exit() effectless - #45620

process,worker: ensure code after exit() effectless#45620
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
ywave620:ensure-exit

ywave620 commented Nov 25, 2022

Uh oh!

ywave620 Nov 25, 2022 •
edited
Loading

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 26, 2022

Uh oh!

nodejs-github-bot commented Nov 27, 2022

Uh oh!

nodejs-github-bot commented Nov 27, 2022

Uh oh!

nodejs-github-bot commented Nov 27, 2022

Uh oh!

ywave620 commented Nov 28, 2022

Uh oh!

ywave620 commented Dec 10, 2022 •
edited
Loading

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 10, 2022

Uh oh!

ywave620 Dec 12, 2022

Uh oh!

nodejs-github-bot commented Dec 12, 2022

Uh oh!

nodejs-github-bot commented Dec 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

+                // i.e. whether v8 exited or is about to exit
+                inline bool terminatedOrTerminating() {
+                  return this->isolate->IsExecutionTerminating() || !can_call_into_js();

process,worker: ensure code after exit() effectless by ywave620 · Pull Request #45620 · nodejs/node · GitHub

Uh oh!

Uh oh!

process,worker: ensure code after exit() effectless - #45620

process,worker: ensure code after exit() effectless#45620nodejs-github-bot merged 5 commits intonodejs:mainnodejs/node:mainfrom ywave620:ensure-exitywave620/node:ensure-exitCopy head branch name to clipboard

Conversation

ywave620 commented Nov 25, 2022

Uh oh!

ywave620 Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Nov 26, 2022

Uh oh!

nodejs-github-bot commented Nov 27, 2022

Uh oh!

nodejs-github-bot commented Nov 27, 2022

Uh oh!

nodejs-github-bot commented Nov 27, 2022

Uh oh!

ywave620 commented Nov 28, 2022

Uh oh!

ywave620 commented Dec 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 10, 2022

Uh oh!

ywave620 Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 12, 2022

Uh oh!

nodejs-github-bot commented Dec 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

process,worker: ensure code after exit() effectless#45620
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
ywave620:ensure-exit

ywave620 Nov 25, 2022 •
edited
Loading

ywave620 commented Dec 10, 2022 •
edited
Loading