{{ message }}
src: refactor tracing code #21867
Closed
addaleax wants to merge 13 commits into
Closed
Conversation
Collaborator
Member
Author
|
Btw, this might not completely resolve the issues with this code – but I think this is an improvement over the current situation. Full CI: https://ci.nodejs.org/job/node-test-pull-request/15929/ |
jasnell
approved these changes
Jul 18, 2018
Contributor
|
Sorry, I do not have a time to do a proper review today. I had been working on an alternative implementation for this (80% done), just want to share design..
|
Member
Author
|
@eugeneo If you’d like to share code, I’m happy to take a look! |
Contributor
|
@addaleax https://github.com/eugeneo/node/tree/single-threaded-tracing - this is my wip branch, but it does not even compile. |
eugeneo
approved these changes
Jul 23, 2018
Contributor
|
I would like to suggest adding a test case, e.g. - https://github.com/eugeneo/node/blob/19379fef2767dcb691b75b432a76bc0715a3ffe1/test/parallel/test-tracing-no-crash.js |
ofrobots
approved these changes
Jul 26, 2018
Contributor
Member
Author
A forward declaration suffices and means that the tracing agent header and its dependencies don’t need to be included in nearly every Node.js file.
Avoid unnecessary copies/extra operations & align with our style guide, and add protection against throwing getters.
Avoid unnecessary operations, add a bit of documentation, and turn the `ClientHandle` smart pointer alias into a real class. This should allow de-coupling the unnecessary combination of a single specific `file_writer` from `Agent`.
The agent code is supposed to manage multiple writers/clients. Adding the default writer into the mix breaks that encapsulation. Instead, manage default options through a separate "virtual" default client handle, and keep the file writer management all to the actual users.
Otherwise this would have crashed the process. In particular, checking the return value of an libuv call against `-1` was invalid to begin with, as libuv uses it to propagate the error code.
Clean up resources when tearing down the tracing agent.
Run the initialization for the file trace writer’s `uv_async_t`s on the same thread as `uv_run()` for their loop to avoid race conditions.
Close existing file descriptors before overriding the field with new ones. Also, use `nullptr` as the loop for these synchronous operations, since they do not run on the same thread as the `uv_run()` call for the previously used loop.
Concurrent writes to the same fd are generally not ideal, since it’s not generally guaranteed that data from those writes will end up on disk in the right order.
addaleax
added a commit
that referenced
this pull request
Aug 1, 2018
PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
addaleax
added a commit
that referenced
this pull request
Aug 1, 2018
Concurrent writes to the same fd are generally not ideal, since it’s not generally guaranteed that data from those writes will end up on disk in the right order. PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
addaleax
pushed a commit
that referenced
this pull request
Aug 1, 2018
PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
A forward declaration suffices and means that the tracing agent header and its dependencies don’t need to be included in nearly every Node.js file. PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
Avoid unnecessary copies/extra operations & align with our style guide, and add protection against throwing getters. PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
Avoid unnecessary operations, add a bit of documentation, and turn the `ClientHandle` smart pointer alias into a real class. This should allow de-coupling the unnecessary combination of a single specific `file_writer` from `Agent`. PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
The agent code is supposed to manage multiple writers/clients. Adding the default writer into the mix breaks that encapsulation. Instead, manage default options through a separate "virtual" default client handle, and keep the file writer management all to the actual users. PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
Otherwise this would have crashed the process. In particular, checking the return value of an libuv call against `-1` was invalid to begin with, as libuv uses it to propagate the error code. PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
Clean up resources when tearing down the tracing agent. PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
Run the initialization for the file trace writer’s `uv_async_t`s on the same thread as `uv_run()` for their loop to avoid race conditions. PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
Close existing file descriptors before overriding the field with new ones. Also, use `nullptr` as the loop for these synchronous operations, since they do not run on the same thread as the `uv_run()` call for the previously used loop. PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
Concurrent writes to the same fd are generally not ideal, since it’s not generally guaranteed that data from those writes will end up on disk in the right order. PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos
pushed a commit
that referenced
this pull request
Aug 1, 2018
PR-URL: #21867 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
This was referenced Aug 16, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

This addresses a number with issues with our tracing code related to code style, readability, race conditions, memory leaks, file descriptor leaks, and other bugs.
I hope that this might help a bit with the flaky test situation around
trace_events.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes