| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Any idea? :) |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
@legendecas Could you run the CI for this? |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
The difference here between !env_->can_call_into_js() and env_->is_stopping() is trivial. Environment::can_call_into_js() also checks if Environment::is_stopping_ is set. Maybe @addaleax can chime in here?
If this line is updated, the lambda variable should also be renamed as perform_call_js_check or similar. Also, I'd find the checks if (!env_->can_call_into_js()) return; below can be merged with this lambda call too.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, semantically, this should be checking for whether we’re currently stopping the current environment through a termination exception, so is_stopping() would seem to be the correct choice?
Sorry, something went wrong.
|
Any thoughts on this PR? |
Sorry, something went wrong.
|
Would you mind updating the PR according to addaleax's comment above? Thanks! |
Sorry, something went wrong.
|
@legendecas addaleax's suggestion does not fix the bug. Please see above for why. I suggest that remove set_can_call_into_js(false) from Environment::ExitEnv(), this is ok because can_call_into_js() also check is_stopping(): Lines 637 to 639 in c203921 |
Sorry, something went wrong.
|
With #45907, env->is_stopping() implies !env->can_call_into_js() on the JavaScript thread. With regard to the comment of #45596 (comment), the current check on if env is stopping is the expected semantic. |
Sorry, something went wrong.
Be more aggresive to clean up the async id stack, and ensure the cleanup when terminating. Calling SetIdle() when terminating is not harmless. When node terminates due to an unhandled exception, v8 preseves the vm state, which is JS and notifies node through PerIsolateMessageListener(). If node calls SetIdle() later, v8 complains because it requires the vm state to either be EXTERNEL or IDLE when embedder calling SetIdle().
|
@legendecas Got you. Changed, PTAL |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks!
Sorry, something went wrong.
Sorry, something went wrong.
|
A test crashed. I wonder if there is a way to get a full stacktrace 🤔️ Failing for the past 1 build (Since [#10087](https://ci.nodejs.org/job/node-test-commit-osx-arm/10087/nodes=osx11/) )
[Took 0.85 sec.](https://ci.nodejs.org/job/node-test-commit-osx-arm/10087/nodes=osx11/testReport/junit/(root)/test/parallel_test_tls_server_verify/history)
Error Message
crashed (-10)
Stacktrace
0 0 connecting with agent1
0 1 connecting with agent2
0 2 connecting with agent3
0 3 connecting with nocert
1 0 connecting with agent1
1 1 connecting with agent2
1 2 connecting with agent3
1 3 connecting with nocert
2 0 connecting with agent1
2 1 connecting with agent2
2 2 connecting with agent3
2 3 connecting with nocert
3 0 connecting with agent1
3 1 connecting with agent2
3 2 connecting with agent3
3 3 connecting with nocert
4 0 connecting with agent1
4 1 connecting with agent2
4 2 connecting with agent3
4 3 connecting with nocert
5 0 connecting with agent1
5 1 connecting with agent2
5 2 connecting with agent3
5 3 connecting with agent4
0 Running 'Do not request certs. Everyone is unauthorized.'
0 - unauthed connection: null
0 - unauthed connection: null
0 2 * unauthed
0 1 * unauthed
0 - unauthed connection: null
0 0 * unauthed
0 - unauthed connection: null
0 3 * unauthed
1 Running 'Allow both authed and unauthed connections with CA1'
1 - unauthed connection: DEPTH_ZERO_SELF_SIGNED_CERT
1 - authed connection: agent1
1 - unauthed connection: UNABLE_TO_VERIFY_LEAF_SIGNATURE
1 1 * unauthed
1 0 * authed
1 2 * unauthed
1 - unauthed connection: UNABLE_TO_GET_ISSUER_CERT
1 3 * unauthed
2 Running 'Do not request certs at connection. Do that later'
2 - connected, renegotiating
2 - authed connection: agent1
2 0 * authed
2 - unauthed connection: null
2 1 * unauthed
2 - unauthed connection: null
2 2 * unauthed
2 - unauthed connection: null
2 3 * unauthed
3 Running 'Allow only authed connections with CA1'
3 - authed connection: agent1
3 0 * authed
4 Running 'Allow only authed connections with CA1 and CA2'
4 - authed connection: agent3
4 - authed connection: agent1
4 2 * authed
4 0 * authed
5 Running 'Allow only certs signed by CA2 but not in the CRL'
|
Sorry, something went wrong.
Sorry, something went wrong.
|
According to the reliability report nodejs/reliability#503, the failed test doesn't seem to be introduced in this PR. I've resumed the build. |
Sorry, something went wrong.
|
@legendecas Could you merge this? |
Sorry, something went wrong.
Be more aggresive to clean up the async id stack, and ensure the cleanup when terminating. Calling SetIdle() when terminating is not harmless. When node terminates due to an unhandled exception, v8 preseves the vm state, which is JS and notifies node through PerIsolateMessageListener(). If node calls SetIdle() later, v8 complains because it requires the vm state to either be EXTERNEL or IDLE when embedder calling SetIdle(). PR-URL: #45596 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Be more aggresive to clean up the async id stack, and ensure the cleanup when terminating. Calling SetIdle() when terminating is not harmless. When node terminates due to an unhandled exception, v8 preseves the vm state, which is JS and notifies node through PerIsolateMessageListener(). If node calls SetIdle() later, v8 complains because it requires the vm state to either be EXTERNEL or IDLE when embedder calling SetIdle(). PR-URL: #45596 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Be more aggresive to clean up the async id stack, and ensure the cleanup when terminating. Calling SetIdle() when terminating is not harmless. When node terminates due to an unhandled exception, v8 preseves the vm state, which is JS and notifies node through PerIsolateMessageListener(). If node calls SetIdle() later, v8 complains because it requires the vm state to either be EXTERNEL or IDLE when embedder calling SetIdle(). PR-URL: #45596 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
| Back | FazBrowse Home | New Git URL |
An inheritor of #45422 and a fix to #43084. I move the original failing test back to the test/parallel/ to increase the chance that it exercises the defect, at which this PR is targeted.
To see the bug in 43084, apply this patch and run the script