-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
tracing: fix static destruction order issue #24123
Conversation
Sometimes, the `parallel/test-tracing-no-crash` would not work as expected, at least on Windows, because there is a static destruction race between tearing down the `NodeTraceWriter` instance and the per-process options struct. If the per-process options were destroyed before the `NodeTraceWriter`, the reference to the tracing filename would be gone before opening the file was attempted. This can be solved by creating a copy of the string when creating the `NodeTraceWriter` instance rather than taking a reference. Fixes: nodejs#22523
@@ -61,7 +61,7 @@ class NodeTraceWriter : public AsyncTraceWriter { | |||
int highest_request_id_completed_ = 0; | |||
int total_traces_ = 0; | |||
int file_num_ = 0; | |||
const std::string& log_file_pattern_; | |||
std::string log_file_pattern_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I've also hit that bug in 6c24c3b
Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/18428/ |
And resume again: https://ci.nodejs.org/job/node-test-pull-request/18466/ |
Resume Build: https://ci.nodejs.org/job/node-test-pull-request/18488/ |
Landed in 71a4cb7 (thanks everyone for kicking off the CI runs!) |
Sometimes, the `parallel/test-tracing-no-crash` would not work as expected, at least on Windows, because there is a static destruction race between tearing down the `NodeTraceWriter` instance and the per-process options struct. If the per-process options were destroyed before the `NodeTraceWriter`, the reference to the tracing filename would be gone before opening the file was attempted. This can be solved by creating a copy of the string when creating the `NodeTraceWriter` instance rather than taking a reference. Fixes: #22523 PR-URL: #24123 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sometimes, the `parallel/test-tracing-no-crash` would not work as expected, at least on Windows, because there is a static destruction race between tearing down the `NodeTraceWriter` instance and the per-process options struct. If the per-process options were destroyed before the `NodeTraceWriter`, the reference to the tracing filename would be gone before opening the file was attempted. This can be solved by creating a copy of the string when creating the `NodeTraceWriter` instance rather than taking a reference. Fixes: #22523 PR-URL: #24123 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sometimes, the `parallel/test-tracing-no-crash` would not work as expected, at least on Windows, because there is a static destruction race between tearing down the `NodeTraceWriter` instance and the per-process options struct. If the per-process options were destroyed before the `NodeTraceWriter`, the reference to the tracing filename would be gone before opening the file was attempted. This can be solved by creating a copy of the string when creating the `NodeTraceWriter` instance rather than taking a reference. Fixes: nodejs#22523 PR-URL: nodejs#24123 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sometimes, the `parallel/test-tracing-no-crash` would not work as expected, at least on Windows, because there is a static destruction race between tearing down the `NodeTraceWriter` instance and the per-process options struct. If the per-process options were destroyed before the `NodeTraceWriter`, the reference to the tracing filename would be gone before opening the file was attempted. This can be solved by creating a copy of the string when creating the `NodeTraceWriter` instance rather than taking a reference. Fixes: #22523 PR-URL: #24123 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sometimes, the `parallel/test-tracing-no-crash` would not work as expected, at least on Windows, because there is a static destruction race between tearing down the `NodeTraceWriter` instance and the per-process options struct. If the per-process options were destroyed before the `NodeTraceWriter`, the reference to the tracing filename would be gone before opening the file was attempted. This can be solved by creating a copy of the string when creating the `NodeTraceWriter` instance rather than taking a reference. Fixes: #22523 PR-URL: #24123 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sometimes, the
parallel/test-tracing-no-crash
would not work asexpected, at least on Windows, because there is a static destruction
race between tearing down the
NodeTraceWriter
instance and theper-process options struct. If the per-process options were destroyed
before the
NodeTraceWriter
, the reference to the tracing filenamewould be gone before opening the file was attempted.
This can be solved by creating a copy of the string when creating the
NodeTraceWriter
instance rather than taking a reference.Fixes: #22523
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes (presumably)