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.

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 #23110

Closed
wants to merge 2 commits into from
Closed

src: ready background workers before bootstrap #23110

wants to merge 2 commits into from

Conversation

ofrobots
Copy link
Contributor

This fixes the thread race on startup with background threads (#23065). Included here is a fix in libuv that needs to land upstream (libuv/libuv#2003), but I am opening the PR early so that I can get the CI to test it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passe
  • 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/1033/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. libuv Issues and PRs related to the libuv dependency or the uv binding. labels Sep 26, 2018
@ofrobots ofrobots force-pushed the fix-windows-background-worker-init branch from 2a5cf52 to 58b2294 Compare September 27, 2018 00:05
@mscdex mscdex added the blocked PRs that are blocked by other issues or PRs. label Sep 27, 2018
@ofrobots ofrobots mentioned this pull request Sep 28, 2018
Closed
3 tasks
@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Sep 28, 2018
@ofrobots
deps: uv: cherry-pick a989af1 from upstream
32bd197 
Original commit message:
  mac,thread: fix pthread_barrier_wait serial thread

  The thread returning PTHREAD_BARRIER_SERIAL_THREAD should be the one
  last to exit the barrier, otherwise it is unsafe to destroy the
  barrier – other threads may have yet to exit.
@ofrobots
src: ready background workers before bootstrap
f0db592 
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 branch from 58b2294 to f0db592 Compare October 1, 2018 22:27
@ofrobots ofrobots changed the title src: ready background workers before bootstra src: ready background workers before bootstrap Oct 1, 2018
@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 1, 2018

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

@ofrobots ofrobots mentioned this pull request Oct 1, 2018
Closed
@joyeecheung joyeecheung mentioned this pull request Oct 2, 2018
Closed
@ofrobots ofrobots mentioned this pull request Oct 2, 2018
Closed
@joyeecheung
Copy link
Member

https://ci.nodejs.org/job/node-test-commit-aix/nodes=aix61-ppc64/18151/console

This seems to be a relevant failure

00:44:54 gmake[1]: *** [test/addons-napi/.buildstamp] IOT/Abort trap

Also failed #22631

@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 2, 2018

I can reproduce the abort on AIX. A core dump gets generated, however, the machine doesn't have gdb or lldb. I cannot get dbx to work:

/u/iojs/ofrobots/node $ dbx ./out/Release/node
Type 'help' for help.
warning: The core file is not a fullcore. Some info may
not be available.
[using memory image in core]
reading symbolic information ...internal error: assertion failed at line 6036 in file object.c
internal error: assertion failed at line 6036 in file object.c
internal error: assertion failed at line 6036 in file object.c
internal error: assertion failed at line 6036 in file object.c
internal error: assertion failed at line 6036 in file object.c
internal error: assertion failed at line 6036 in file object.c
internal error: assertion failed at line 6036 in file object.c
internal error: assertion failed at line 6036 in file object.c
dbx: fatal error: Not enough space

This is not about disk space, as there is plenty of disk space on the machine.

@nodejs/platform-aix is there any help you can provide? Would it make sense for build machines to have standard debuggers (gdb, lldb) available by default?

@joyeecheung
Copy link
Member

@ofrobots I ran into similar issues on AIX before and I think it's caused by "not enough space in /tmp" (@refack was the one who got it fixed)

@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 3, 2018 •
edited

Thanks @joyeecheung, that is probably it. I was able to make progress by sprinkling printfs everywhere.

Aside @nodejs/platform-aix @nodejs/build it seems that we only support AIX >= 7. It seems that AIX 6 is already end-of-life by IBM. Is there a reason why the CI is continuing to test against AIX 6?

@ofrobots ofrobots mentioned this pull request Oct 3, 2018
Closed
2 tasks
@richardlau
Copy link
Member

Aside @nodejs/platform-aix @nodejs/build it seems that we only support AIX >= 7. It seems that AIX 6 is already end-of-life by IBM. Is there a reason why the CI is continuing to test against AIX 6?

Because Node.js v6.x is still around until next April and theoretically is supported on AIX 6.1 TL09. In an ideal world we'd have more AIX images in the CI. There are some code paths in libuv on AIX that we currently aren't testing because they're only available in AIX 7 onwards.

cc @gdams

@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 3, 2018

Superseded by #23233.

@ofrobots ofrobots closed this Oct 3, 2018
@ofrobots ofrobots deleted the fix-windows-background-worker-init branch October 3, 2018 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. c++ Issues and PRs that require attention from people who are familiar with C++. libuv Issues and PRs related to the libuv dependency or the uv binding. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ofrobots @nodejs-github-bot @joyeecheung @richardlau @jasnell @mscdex