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

src: ready background workers before bootstrap #23233

Closed
wants to merge 3 commits into from
Closed

src: ready background workers before bootstrap #23233

wants to merge 3 commits into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Oct 3, 2018 •
edited
Loading

This is an alternative to #23110 that doesn't depend upon libuv/libuv#2003.

Make sure background workers are ready before proceeding with the
bootstrap or post-bootstrap execution of any code that may trigger
process.exit().

Fixes: #23065

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@ofrobots build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1108/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 3, 2018
bnoordhuis
bnoordhuis reviewed Oct 3, 2018
View reviewed changes
src/node_platform.cc Outdated Show resolved Hide resolved
@joyeecheung joyeecheung mentioned this pull request Oct 3, 2018
Closed
@ofrobots
src: ready background workers before bootstrap
3afca3a 
Make sure background workers are ready before proceeding with the
bootstrap or post-bootstrap execution of any code that may trigger
`process.exit()`.

Fixes: nodejs#23065
@ofrobots ofrobots force-pushed the fix-windows-background-worker-init-mutex branch from de3e607 to 3afca3a Compare October 3, 2018 17:24
@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 3, 2018

Switched to C++ mutex classes. This needs another look.

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

@ofrobots ofrobots mentioned this pull request Oct 3, 2018
Closed
2 tasks
@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 3, 2018

Relaunched SmartOS: https://ci.nodejs.org/job/node-test-commit-smartos/20672/

@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 3, 2018

SmartOS failures are in parallel/test-stream-buffer-list, which got fixed in #23232.

addaleax
addaleax reviewed Oct 3, 2018
View reviewed changes
src/node_platform.cc Outdated Show resolved Hide resolved
@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 4, 2018

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

@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 4, 2018

Re-run of arm-fanned, as there seemed to be unrelated failures: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3841/

@Trott
Copy link
Member

Trott commented Oct 6, 2018

Is there any chance test-stream-buffer-list failing on all three Pi variants is remotely related?

21:30:07 not ok 228 parallel/test-stream-buffer-list
21:30:07   ---
21:30:07   duration_ms: 1.35
21:30:07   severity: fail
21:30:07   exitcode: 1
21:30:07   stack: |-
21:30:07     assert.js:84
21:30:07       throw new AssertionError(obj);
21:30:07       ^
21:30:07     
21:30:07     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
21:30:07     + actual - expected
21:30:07     
21:30:07     + �[32m'BufferList { head: \u001b[1mnull\u001b[22m, tail: \u001b[1mnull\u001b[22m, length: \u001b[33m0\u001b[39m }'�[39m
21:30:07     - �[32m'BufferList { length: \u001b[33m0\u001b[39m }'�[39m
21:30:07         at Object.<anonymous> (/home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-stream-buffer-list.js:37:8)
21:30:07         at Module._compile (internal/modules/cjs/loader.js:703:30)
21:30:07         at Object.Module._extensions..js (internal/modules/cjs/loader.js:714:10)
21:30:07         at Module.load (internal/modules/cjs/loader.js:613:32)
21:30:07         at tryModuleLoad (internal/modules/cjs/loader.js:552:12)
21:30:07         at Function.Module._load (internal/modules/cjs/loader.js:544:3)
21:30:07         at Function.Module.runMain (internal/modules/cjs/loader.js:756:12)
21:30:07         at startup (internal/bootstrap/node.js:303:19)
21:30:07         at bootstrapNodeJSCore (internal/bootstrap/node.js:865:3)
21:30:07   ...

@Trott
Copy link
Member

Trott commented Oct 6, 2018

^^^^^ Guessing not since it didn't fail the first time? Let's try a rebuild again...

https://ci.nodejs.org/job/node-test-commit-arm-fanned/3868/

@Trott
Copy link
Member

Trott commented Oct 6, 2018

Yikes, infra that last time.

Trying again: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3869/

@Trott
Copy link
Member

Trott commented Oct 6, 2018

I see the problem. The rebuilds don't rebase unless you tell them what commit to rebase onto, so they don't have a fix for that one test from a few days ago. Here's a rebuild with a rebase on current master: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3870/

@Trott
Copy link
Member

Trott commented Oct 6, 2018

Landed in e273abc

@Trott Trott closed this Oct 6, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 6, 2018
@ofrobots @Trott
src: ready background workers before bootstrap
e273abc 
Make sure background workers are ready before proceeding with the
bootstrap or post-bootstrap execution of any code that may trigger
`process.exit()`.

Fixes: nodejs#23065

PR-URL: nodejs#23233
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos
Copy link
Member

targos commented Oct 6, 2018

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@ofrobots ofrobots mentioned this pull request Oct 10, 2018
Closed
@ofrobots ofrobots deleted the fix-windows-background-worker-init-mutex branch October 10, 2018 05:45
@ofrobots
Copy link
Contributor Author

v10.x backport: #23398

ofrobots added a commit to ofrobots/node that referenced this pull request Oct 13, 2018
@ofrobots
src: ready background workers before bootstrap
63f0e77 
Make sure background workers are ready before proceeding with the
bootstrap or post-bootstrap execution of any code that may trigger
`process.exit()`.

Fixes: nodejs#23065

PR-URL: nodejs#23233
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
ofrobots added a commit that referenced this pull request Oct 15, 2018
@ofrobots
src: ready background workers before bootstrap
5fea320 
Make sure background workers are ready before proceeding with the
bootstrap or post-bootstrap execution of any code that may trigger
`process.exit()`.

Fixes: #23065

PR-URL: #23233
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@ofrobots @jasnell
src: ready background workers before bootstrap
c2fee5d 
Make sure background workers are ready before proceeding with the
bootstrap or post-bootstrap execution of any code that may trigger
`process.exit()`.

Fixes: #23065

PR-URL: #23233
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@ofrobots @MylesBorins
src: ready background workers before bootstrap
25d7c02 
Make sure background workers are ready before proceeding with the
bootstrap or post-bootstrap execution of any code that may trigger
`process.exit()`.

Fixes: #23065

PR-URL: #23233
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@ofrobots @rvagg
src: ready background workers before bootstrap
8149702 
Make sure background workers are ready before proceeding with the
bootstrap or post-bootstrap execution of any code that may trigger
`process.exit()`.

Fixes: #23065

PR-URL: #23233
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@ofrobots @MylesBorins
src: ready background workers before bootstrap
c854879 
Make sure background workers are ready before proceeding with the
bootstrap or post-bootstrap execution of any code that may trigger
`process.exit()`.

Fixes: #23065

PR-URL: #23233
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
@gireeshpunathil gireeshpunathil mentioned this pull request Dec 22, 2018
Closed
@addaleax addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. v8 platform Issues and PRs related to Node's v8::Platform implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Platform thread race: process.exit executed before background threads are ready
7 participants
@ofrobots @nodejs-github-bot @Trott @targos @bnoordhuis @jasnell @addaleax