-
Notifications
You must be signed in to change notification settings - Fork 71
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
DCHECK crash on !i_isolate->is_execution_terminating()
#227
Comments
Thanks for opening this. It also affect V8 10.3: nodejs/node#43195 (comment) |
Sorry for the inconvenience here :/. I think V8 has been a bit too lenient on this in the past (and it led to a bunch of issues within chrome). I think having simple accessors still available would make sense (and it might well be that I totally missed a use case present in node). |
Example in CI: https://ci.nodejs.org/job/node-test-commit-arm-debug/2497/nodes=ubuntu1804-arm64/ Problematic code: https://github.com/nodejs/node/blob/6ac55fa337b57cecf36602d0acc3da25ee82a589/src/env-inl.h#L222-L237 /cc @nodejs/async_hooks |
Adding |
I’m not sure what else to do besides doing this and being consistent about this. |
Problem seems to be the perform_stopping_check() lambda in callback.cc Close method. It tries to clean up async_hooks reactively to the env stopping, but at that point it's no longer safe to create the HandleScope for setting the length field on js_execution_async_resources_. If the env is already in shutdown, it should be safe to just skip that though. 🤔 I suspect we're going to need to just play that game of whack-a-mole to get any of those post-shutdown stuff filtered out. |
Alright - i can spin up a PR for that soon then! |
Hey 👋 I tried to reproduce the bug, but I struggled a bit as I'm fairly new to the codebase. I have two questions:
|
@tony-go tests can be run as a rule with just the node executable, so |
Thanks, @codebytere 🙌 Finally found this: https://github.com/nodejs/node/blob/main/BUILDING.md#running-tests |
@Flarna hmm - that's strange since theres a |
There's a bunch of places within that function that the state can transition though, which is why it checks it several times. Probably another place it needs a check now or something...it's a complicated bit of code. 😐 |
@codebytere It's a while that I had this in my debugger so can't remember in detail. But as far as I remmeber it's as @Qard said the state changes after that check you refer to. |
I know the microtask queue bits change it briefly. Forget if there were other cases, but basically these callback scopes can end up nested within each other and that can have some unexpected side effects if not handled carefully. |
This commit fixes the issue for me. Should I open a PR in this repo or somewhere else? |
@santigimeno you can open a PR on nodejs/node, targeting the |
Done. Thanks @targos |
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Looks like we're still having issues - when we run
Which looks like a problem with cc @santigimeno |
@codebytere by looking at the stacktrace I'm not sure it is related to the patch. Regardless, what should I do to reproduce this locally? I can't see any crashes when running it with neither the node.js main branch nor the canary branch. |
@santigimeno does it not happen against canary when running
these two calls to Seems to be related to https://chromium-review.googlesource.com/c/chromium/src/+/3620285 |
Here's the latest canary CI run in debug mode (green): https://ci.nodejs.org/job/node-test-commit-arm-debug/4183/nodes=ubuntu1804-arm64/ Maybe these crashes are only reproducible within Electron? |
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Looks like it 🤔 at least, it looks like it's specific to embedded Node.js and i haven't been able to repro it on your canary branch Tracked it down to https://github.com/electron/node/blob/3e6b3b22c5f687e170944a1cef51caa41a4caea1/src/node_task_queue.cc#L142-L143 - it's causing this error:
|
@santigimeno i determined it's related to also cc @addaleax since this appears specific to timeouts with |
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: #44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: nodejs/node#44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Fix multiple instances of those uncovered while running the tests on debug builds. Fixes: nodejs/node-v8#227 PR-URL: nodejs/node#44669 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
https://chromium-review.googlesource.com/c/v8/v8/+/3620285
was merged recently and causes a ton of failures in a variety of tests (~50 or so,
parallel/test-worker-esm-exit
as an example):I initially attempted to fix by adding in early returns in various places:
but it ended up being a bit of a whac-a-mole game with new ones appearing with every early return I added so i'm not sure what the best approach is.
cc @targos and @camillobruni since you opened the CL :)
The text was updated successfully, but these errors were encountered: