src: shutdown libuv before exit() #35021

addaleax · 2020-09-02T12:59:58Z

This ensures that no operations will be running on the libuv
threadpool, which is important because they may run into race
conditions with the global destructors being triggered from
exit(), such as in the added test example here.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

jasnell · 2020-09-02T13:02:39Z

We should fast track this. It also needs to be backported to 14.x and 12.x. haven't tried 10.x but likely there too.

test/parallel/test-crypto-op-during-process-exit.js

nodejs-github-bot · 2020-09-02T13:32:39Z

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

nodejs-github-bot · 2020-09-02T13:49:50Z

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

nodejs-github-bot · 2020-09-02T13:58:45Z

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

jasnell · 2020-09-02T14:32:57Z

I'll investigate shortly why it's failing on windows

jasnell · 2020-09-02T15:20:22Z

Ok... well... the failure on windows is still a segfault in the openssl EVP_PKEY_keygen call so this doesn't appear that it is fixing that problem, unfortunately. The uv_library_shutdown() call is happening...

I instrumented with some printf statements... here's what I'm seeing:

ENTERED EVP_PKEY_keygen is printed from immediately inside EVP_PKEY_keygen() when it is called. If that function exits, there's supposed to be a corresponding EXITING EVP_PKEY_keygen statement.

The uv_library_shutdown statement is printed immediately before the added call to uv_library_shutdown() that this PR adds.

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
0

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
0

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
-1073741819

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
0

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
-1073741819

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
0

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
-1073741819

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
-1073741819

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
-1073741819

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
-1073741819

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
0

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
0

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
uv_library_shutdown
0

Now, that said, it does appear to segfault occasionally after EVP_PKEY_keygen exits ... (the 5 in the output below simply identifies which return statement inside EVP_PKEY_keygen ran

C:\Users\jasne\Projects\node>Release\node test\parallel\test-crypto-op-during-process-exit & echo %errorlevel%
ENTERED EVP_PKEY_keygen
EXITING EVP_PKEY_keygen 5
uv_library_shutdown
-1073741819

jasnell · 2020-09-02T15:48:13Z

Ok, there are two distinct failure patterns that I'm seeing here:

Segfault sometime after the uv_library_shutdown() call when EVP_PKEY_keygen() is running and...

Segfault sometime after the CryptoJob completes and is deconstructed, but always after the uv_library_shutdown() call.

jasnell · 2020-09-02T15:57:36Z

I'm attempting to do a debug build on windows now. It's going to take a while and I'm not sure if it's going to build successfully. The last time I tried a debug build on windows it failed....

jasnell · 2020-09-02T16:33:09Z

Ok. So, yeah, at least on Windows this PR is not fixing the problem. windbg is reporting Invalid access exception at...

jasnell · 2020-09-02T18:58:06Z

Ok, libuv PR open here: libuv/libuv#2983

This PR should also land but the libuv fix will be necessary for the test case here to pass on windows. @addaleax, we can keep this PR open until we get the libuv fix, or, we can perhaps move the test into the known_issues set and get this landed now. Once the libuv fix lands here, we can move the test back into parallel. What do you think?

addaleax · 2020-09-02T19:15:49Z

we can perhaps move the test into the known_issues set and get this landed now.

The problem with that is that it will not crash reliably anywhere, and currently only does so on Windows… we could just skip the test on Windows if you like, and undo that when the libuv fix lands? Does that sound okay? (If yes, please push to this branch directly, I’ll be away for ~24 hours :))

jasnell · 2020-09-03T02:50:25Z

Yeah that sounds good. I'll push that in the morning

nodejs-github-bot · 2020-09-03T16:15:23Z

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

juanarbol

LGTM

nodejs-github-bot · 2020-09-05T11:46:16Z

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

Trott · 2020-09-05T22:15:07Z

Landed in e326c41

This ensures that no operations will be running on the libuv threadpool, which is important because they may run into race conditions with the global destructors being triggered from `exit()`, such as in the added test example here. PR-URL: nodejs#35021 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This ensures that no operations will be running on the libuv threadpool, which is important because they may run into race conditions with the global destructors being triggered from `exit()`, such as in the added test example here. PR-URL: #35021 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This ensures that no operations will be running on the libuv threadpool, which is important because they may run into race conditions with the global destructors being triggered from `exit()`, such as in the added test example here. PR-URL: nodejs#35021 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Fixes: libuv#2980 Refs: nodejs/node#35021 Signed-off-by: James M Snell <jasnell@gmail.com>

Fixes: #2980 Refs: nodejs/node#35021 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jameson Nash <vtjnash@gmail.com>

Fixes: libuv#2980 Refs: nodejs/node#35021 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jameson Nash <vtjnash@gmail.com>

addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. lts-watch-v10.x labels Sep 2, 2020

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 2, 2020

jasnell approved these changes Sep 2, 2020

View reviewed changes

richardlau reviewed Sep 2, 2020

View reviewed changes

test/parallel/test-crypto-op-during-process-exit.js Outdated Show resolved Hide resolved

addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 2, 2020

devsnek approved these changes Sep 2, 2020

View reviewed changes

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020

santigimeno approved these changes Sep 2, 2020

View reviewed changes

cjihrig approved these changes Sep 2, 2020

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 2, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020

richardlau approved these changes Sep 2, 2020

View reviewed changes

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020

richardlau approved these changes Sep 2, 2020

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020

addaleax removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Sep 2, 2020

devnexen approved these changes Sep 3, 2020

View reviewed changes

himself65 approved these changes Sep 3, 2020

View reviewed changes

Flarna approved these changes Sep 3, 2020

View reviewed changes

addaleax added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 3, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2020

vtjnash mentioned this pull request Sep 4, 2020

call uv_library_shutdown explicitly before exit JuliaLang/julia#37400

Closed

juanarbol approved these changes Sep 5, 2020

View reviewed changes

Trott approved these changes Sep 5, 2020

View reviewed changes

Trott force-pushed the uv-shutdown branch from 45dedbb to e326c41 Compare September 5, 2020 22:15

Trott merged commit e326c41 into nodejs:master Sep 5, 2020

richardlau mentioned this pull request Sep 7, 2020

v14.10.0 proposal #35023

Merged

4 tasks

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

addaleax deleted the uv-shutdown branch October 14, 2020 12:45

MylesBorins removed the lts-watch-v12.x label Nov 16, 2020

bnoordhuis pushed a commit to jasnell/libuv that referenced this pull request Jan 23, 2021

Remove the _WIN32 guards from uv__threadpool_cleanup

6da0a39

Fixes: libuv#2980 Refs: nodejs/node#35021 Signed-off-by: James M Snell <jasnell@gmail.com>

lovell mentioned this pull request Feb 24, 2023

Is it possible to get (or compile) .pdb file for sharp.node for specfic sharp version? lovell/sharp#3569

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: shutdown libuv before exit() #35021

src: shutdown libuv before exit() #35021

addaleax commented Sep 2, 2020

jasnell commented Sep 2, 2020

nodejs-github-bot commented Sep 2, 2020

nodejs-github-bot commented Sep 2, 2020

nodejs-github-bot commented Sep 2, 2020

jasnell commented Sep 2, 2020

jasnell commented Sep 2, 2020 •

edited

Loading

jasnell commented Sep 2, 2020

jasnell commented Sep 2, 2020

jasnell commented Sep 2, 2020 •

edited

Loading

jasnell commented Sep 2, 2020

addaleax commented Sep 2, 2020

jasnell commented Sep 3, 2020

nodejs-github-bot commented Sep 3, 2020

juanarbol left a comment

nodejs-github-bot commented Sep 5, 2020

Trott commented Sep 5, 2020

src: shutdown libuv before exit() #35021

src: shutdown libuv before exit() #35021

Conversation

addaleax commented Sep 2, 2020

Checklist

jasnell commented Sep 2, 2020

nodejs-github-bot commented Sep 2, 2020

nodejs-github-bot commented Sep 2, 2020

nodejs-github-bot commented Sep 2, 2020

jasnell commented Sep 2, 2020

jasnell commented Sep 2, 2020 • edited Loading

jasnell commented Sep 2, 2020

jasnell commented Sep 2, 2020

jasnell commented Sep 2, 2020 • edited Loading

jasnell commented Sep 2, 2020

addaleax commented Sep 2, 2020

jasnell commented Sep 3, 2020

nodejs-github-bot commented Sep 3, 2020

juanarbol left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 5, 2020

Trott commented Sep 5, 2020

jasnell commented Sep 2, 2020 •

edited

Loading

jasnell commented Sep 2, 2020 •

edited

Loading