fix: do not crash when async id check fails #18452
Conversation
|
A maintainer has manually backported this PR to "6-0-x", please check out #18453 |
|
A maintainer has manually backported this PR to "5-0-x", please check out #18454 |
1 similar comment
|
A maintainer has manually backported this PR to "5-0-x", please check out #18454 |
|
A maintainer has manually backported this PR to "4-2-x", please check out #18455 |
1 similar comment
|
A maintainer has manually backported this PR to "4-2-x", please check out #18455 |
|
A maintainer has manually backported this PR to "3-1-x", please check out #18456 |
1 similar comment
|
A maintainer has manually backported this PR to "3-1-x", please check out #18456 |
nornagon
left a comment
nornagon
left a comment
There was a problem hiding this comment.
What are the consequences of the failing check? Should we disable async hooks altogether if they don't work correctly?
| it('does not crash', () => { | ||
| const asyncHooks = require('async_hooks') | ||
| const hook = asyncHooks.createHook({ init: function () {} }) | ||
| hook.enable() |
There was a problem hiding this comment.
can we disable this after the test is done?
There was a problem hiding this comment.
I have updated the PR, will update the back ports after this gets merged.
The failing checks does not affect runtime code, the async IDs are just numbers provided to track executions of async calls. In the worst case, users using Node started to enable the check by default from Node 10 (nodejs/node#16318), this PR essentially reverts the change and make Electron's behavior aligned with Electron 2.0, so it shouldn't make things worse. Disabling async hooks altogether sounds good to me, but I'm not sure what's the best way, should It is probably a change that should be separated from this PR. |
zcbenz
force-pushed
the
uv-run-once-corruption
branch
from
cdc4ea0 to
3090d89
Compare
|
I'm concerned that just failing to crash in this situation will cause more subtle and hard-to-detect failures later on as the async hook stack gets corrupted. I think we need a real fix for this, I'm not convinced that merging this will truly improve the situation. |
Description of Change
Close #12798.
Due to the way node integration works in Electron, the async hooks can not correctly track the execution of async calls under certain cases. We should eventually find out how to make async hooks work correctly in Electron, but for now we just disable the check to avoid hard crashes.
The async hooks will not be able to track those executions which crashed before, but there are no other side effects.
Checklist
npm testpassesRelease Notes
Notes: Fix crash when throwing Error in
setImmediate