Skip to content

src: ready background workers before bootstrap#23233

Closed
ofrobots wants to merge 3 commits intonodejs:masterfrom
ofrobots:fix-windows-background-worker-init-mutex
Closed

src: ready background workers before bootstrap#23233
ofrobots wants to merge 3 commits intonodejs:masterfrom
ofrobots:fix-windows-background-worker-init-mutex

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

nodejs-github-bot commented Oct 3, 2018

@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
@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
@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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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

ofrobots commented Oct 10, 2018

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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax 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