src: remove debugger dead code #12621
Conversation
nodejs-github-bot
added
build
targos
removed
build
|
It seems like this is causing a problem on 32-bit platforms |
| @@ -25,6 +25,7 @@ | |||
| #include "node_crypto.h" | |||
| #include "node_crypto_bio.h" | |||
| #include "node_crypto_groups.h" | |||
| #include "node_mutex.h" | |||
There was a problem hiding this comment.
The file needs this #include. It was working because debug-agent.h had it.
|
On 32 posix Maybe you left an uninitialized variable that when exiting trying to free it segfaults? |
| @@ -25,6 +25,7 @@ | |||
| #include "node_crypto.h" | |||
| #include "node_crypto_bio.h" | |||
| #include "node_crypto_groups.h" | |||
| #include "node_mutex.h" | |||
[expand for backtrace & register info]Some Unfortunately, this only seems to happen in Release mode. and while single-stepping through the destructor in a debugger the segfault did not show, so it might be a race condition… :/ I might have some time to help looking deeper into it later today. |
|
Okay, I think I have an idea of what’s going on. The test fixtures create multiple This is enough to fix the tests for me: diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc
index a7579ac11671..79bfbf50b45c 100644
--- a/test/cctest/test_environment.cc
+++ b/test/cctest/test_environment.cc
@@ -42,6 +42,7 @@ class EnvironmentTest : public NodeTestFixture {
~Env() {
FreeIsolateData(isolate_data_);
+ environment_->CleanupHandles();
FreeEnvironment(environment_);
}But it makes me wonder, should (edit: sorry @targos if this is spoiling the fun of debugging here 😄) |
Sorry, I missed this issue completely. I was not aware of |
Or maybe part of the Anyway, thank you for investigating this @addaleax! I've applied your patch for now. |
See #12344 for why the answer to that question unfortunately is 'no'. |
|
Windows 10, VS2015x64, one test fails
|
|
Re running just windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/8635/ |
| @@ -42,6 +42,7 @@ class EnvironmentTest : public NodeTestFixture { | |||
|
|
|||
| ~Env() { | |||
| FreeIsolateData(isolate_data_); | |||
| environment_->CleanupHandles(); | |||
There was a problem hiding this comment.
I would suggest landing this in a separate commit, it’s a logically not part of the rest of the changes, and we probably want to backport it (e.g. as part of #12664)
PR-URL: nodejs#12621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
The test fixtures create multiple node::Environments that all use the uv_default_loop(), and since the test does not clean up the handles created by Environment::Start(), the default libuv loop structure contains dangling pointers after the first Environment is freed, which then means that creating new handles leads to memory corruption. PR-URL: nodejs#12621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
|
@addaleax I kept the test fix as a separate commit and set you as the author since it's your finding and I used your very good explanation for the commit message. |
|
ping @addaleax |
|
@targos Ah, sorry, missed the ping! Yes, this is perfectly fine :) |
The test fixtures create multiple node::Environments that all use the uv_default_loop(), and since the test does not clean up the handles created by Environment::Start(), the default libuv loop structure contains dangling pointers after the first Environment is freed, which then means that creating new handles leads to memory corruption. PR-URL: #12621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #12621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
The test fixtures create multiple node::Environments that all use the uv_default_loop(), and since the test does not clean up the handles created by Environment::Start(), the default libuv loop structure contains dangling pointers after the first Environment is freed, which then means that creating new handles leads to memory corruption. PR-URL: nodejs#12621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
According to the explanation in nodejs#3635#issuecomment-157714683 And as a continuation to nodejs#5422 we also ignore EMFILE "No more file descriptors are available,so no more files can be opened" PR-URL: nodejs#12698 Fixes: nodejs#10286 Refs: nodejs#3635 (comment) Refs: nodejs#5178 Refs: nodejs#5179 Refs: nodejs#4005 Refs: nodejs#5121 Refs: nodejs#5422 Refs: nodejs#12621 (comment) Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
According to the explanation in #3635#issuecomment-157714683 And as a continuation to #5422 we also ignore EMFILE "No more file descriptors are available,so no more files can be opened" PR-URL: #12698 Fixes: #10286 Refs: #3635 (comment) Refs: #5178 Refs: #5179 Refs: #4005 Refs: #5121 Refs: #5422 Refs: #12621 (comment) Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
/cc @bnoordhuis
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
debugger