-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Investigate flaky test-worker-init-failure #33759
Comments
Seeing this flake a bunch locally while preparing a 14.x release |
Happened in node-daily-master today. https://ci.nodejs.org/job/node-test-commit-osx/35263/nodes=osx1015/console
|
cc @nodejs/workers @HarshithaKP |
I think this might be specific to macOS. If somebody can reproduce locally, I’m up for a joint debugging session. |
I will try to reproduce it. |
@addaleax I can reproduce this somewhat reliably if you want to pair. |
Showed up in node-daily-master again today. https://ci.nodejs.org/job/node-test-commit-osx/35280/nodes=osx1015/console
|
node/test/parallel/test-worker-init-failure.js Lines 37 to 40 in bf0d82c
I think the assumption that only
Accommodating this error code too in the assertion seems to a pragmatic approach? |
Accommodating a helpful ENOENT error message would indeed be pragmatic. Does // Test that workers fail with meaningful error message
// when their initialization fails. I'm conflicted about whether the right thing to do is to accommodate this error message or if the right thing to do is to try to improve it somehow. |
@Trott - agree, makes sense! I think part of it has also to do with a limitation in libuv: it does not tell which file was missing. We had discussions on that in a different issue, but there wasn't any solution. |
Ref: #11520 |
I would suggest that it’s worth figuring out why libuv is failing with this error, particularly because it’s OS-dependent. @MylesBorins I’m available most of next week, maybe we can find some time then :) |
@addaleax next week is tough for me due to TC39. Ping me on chat and maybe we can work something out |
Happened in node-daily-master again. https://ci.nodejs.org/job/node-test-commit-osx/35435/nodes=osx1015/console
Hopefully @addaleax and @MylesBorins can pair to debug this soon, and if not (or even if so) maybe someone on @nodejs/platform-macos is able to reproduce and/or debug. |
other than the API call ( node/src/inspector_profiler.cc Line 400 in c61b388
though I am unable to reproduce this, I will see if this line is relevant for the test, and possible reasons for this to fail |
I’m assuming you mean |
yes, |
this is the definite path that leads upto at process.wrappedCwd [as cwd] (internal/bootstrap/switches/does_own_process_state.js:128:3)
at Object.resolve (path.js:978:47)
at patchProcessObject (internal/bootstrap/pre_execution.js:101:28)
at prepareMainThreadExecution (internal/bootstrap/pre_execution.js:25:3)
at internal/main/run_main_module.js:7:1 and 85814/0x4cc3f2: open_nocancel(".\0", 0x0, 0x1) = 3 0
85814/0x4cc3f2: fstat64(0x3, 0x7FFEE75254E0, 0x0) = 0 0
85814/0x4cc3f2: fcntl_nocancel(0x3, 0x32, 0x7FFEE7525050) = 0 0
85814/0x4cc3f2: close_nocancel(0x3) = 0 0
85814/0x4cc3f2: stat64("/Users/gireeshpunathil/Desktop\0", 0x7FFEE7525450, 0x0) = 0 0 the most relevant one in this context is So it is evident that a worker is prone to fail in this manner when run under restricted resources (in this case |
Yeah, that makes sense – I think it would be nice to wrap the |
Absorb low level libuv failure in the process initialization phase Fixes: #33759 Refs: #33759 (comment) PR-URL: #34519 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Still failing on CI on macOS. https://ci.nodejs.org/job/node-test-commit-osx/35686/nodes=osx1015/console
|
I wonder if the right thing to do is remove the |
Let the check for the error code suffice and don't check for a particular string in the message. Fixes: nodejs#33759
looks like we have one more case that we missed here: the exact same So if we accept the previous fix in principle, the right thing is to accommodate this error type in the test case itself. /cc @addaleax for opinion. |
o! failed to see @Trott 's PR. will review that instead. |
Absorb low level libuv failure in the process initialization phase Fixes: #33759 Refs: #33759 (comment) PR-URL: #34519 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Absorb low level libuv failure in the process initialization phase Fixes: #33759 Refs: #33759 (comment) PR-URL: #34519 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Harshitha K P <harshitha014@gmail.com>
The text was updated successfully, but these errors were encountered: