src: refactor tracing code #21867

addaleax · 2018-07-18T10:20:29Z

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), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2018-07-18T10:20:30Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/302/pipeline

addaleax · 2018-07-18T12:37:06Z

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/

eugeneo · 2018-07-18T17:17:36Z

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..

File writer and inspector writers live entirely on a "tracing thread". I.e. no asyncs, no mutexes, etc.
node_trace_buffer is fully thread safe (all calls are synchronized).
tracing/agent.cc is the one that passes buffers from "any" thread (i.e. the thread that trace controller chose to call the buffer) to the writers on the tracing thread.
inspector protocol writer uses MainThreadInterface to pass the message from the tracing thread to the inspector dispatcher on the main thread.

addaleax · 2018-07-18T17:50:52Z

@eugeneo If you’d like to share code, I’m happy to take a look!

eugeneo · 2018-07-19T06:14:13Z

@addaleax https://github.com/eugeneo/node/tree/single-threaded-tracing - this is my wip branch, but it does not even compile.

eugeneo · 2018-07-23T02:24:25Z

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 · 2018-07-26T00:41:56Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/16009/

addaleax · 2018-07-27T17:38:44Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/16033/

(edit: Windows failures are real)

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.

PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

Fixes: #22042 PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

Avoid unnecessary copies/extra operations & align with our style guide, and add protection against throwing getters. PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

Clean up resources when tearing down the tracing agent. PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

Fixes: #22042 PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

addaleax requested review from eugeneo, jasnell and ofrobots July 18, 2018 10:20

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 18, 2018

addaleax added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jul 18, 2018

jasnell approved these changes Jul 18, 2018
View reviewed changes

eugeneo approved these changes Jul 23, 2018
View reviewed changes

ofrobots approved these changes Jul 26, 2018
View reviewed changes

addaleax force-pushed the tracing branch from 17fc171 to 0bbe35aCompare July 27, 2018 17:35

addaleax mentioned this pull request Jul 31, 2018
Segfault when spawning child with trace events enabled #22042
Closed

addaleax added 11 commits August 1, 2018 15:11

src: reduce unnecessary includes
2cd8e37
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.

src: minor refactor of node_trace_events.cc
0bb51a5
Avoid unnecessary copies/extra operations & align with our style guide, and add protection against throwing getters.

src: refactor tracing agent code
b688231
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`.

src: fix tracing if cwd or file path is inaccessible
f030398
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.

src: close tracing event loop
58e6413
Clean up resources when tearing down the tracing agent.

src: initialize file trace writer on tracing thread
cf9734c
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.

src: plug trace file file descriptor leak
94c0449
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.

src: use unique_ptr for internal JSON trace writer
fc85207

src: use only one tracing write fs req at a time
2fb4258
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.

src: clean up agent loop when exiting through destructor
74efd1f

addaleax added a commit that referenced this pull request Aug 1, 2018
src: use unique_ptr for internal JSON trace writer
25fdab5
PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

addaleax pushed a commit that referenced this pull request Aug 1, 2018
test: add tracing crash regression test
55623b5
PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

addaleax deleted the tracing branch August 1, 2018 15:28

targos pushed a commit that referenced this pull request Aug 1, 2018
src: use unique_ptr for internal JSON trace writer
6b58746
PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

targos pushed a commit that referenced this pull request Aug 1, 2018
test: add tracing crash regression test
c2372ea
PR-URL: #21867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

rvagg mentioned this pull request Aug 13, 2018
Release proposal: v10.9.0 #22295
Merged

renovatebot mentioned this pull request Feb 8, 2019
Update Node.js to v10.9.0 mozilla/treeherder#3929
Merged

renovatebot mentioned this pull request Feb 19, 2019
chore(deps): update node.js to v10.9.0 jaypeng2015/aws-this-week#58
Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

src: refactor tracing code #21867

src: refactor tracing code #21867

Uh oh!

addaleax commented Jul 18, 2018

Uh oh!

nodejs-github-bot commented Jul 18, 2018

Uh oh!

addaleax commented Jul 18, 2018

Uh oh!

eugeneo commented Jul 18, 2018

Uh oh!

addaleax commented Jul 18, 2018

Uh oh!

eugeneo commented Jul 19, 2018

Uh oh!

eugeneo commented Jul 23, 2018

Uh oh!

ofrobots commented Jul 26, 2018

Uh oh!

addaleax commented Jul 27, 2018•
edited
Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

src: refactor tracing code #21867

src: refactor tracing code #21867

Uh oh!

Conversation

addaleax commented Jul 18, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jul 18, 2018

Uh oh!

addaleax commented Jul 18, 2018

Uh oh!

eugeneo commented Jul 18, 2018

Uh oh!

addaleax commented Jul 18, 2018

Uh oh!

eugeneo commented Jul 19, 2018

Uh oh!

eugeneo commented Jul 23, 2018

Uh oh!

ofrobots commented Jul 26, 2018

Uh oh!

addaleax commented Jul 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

addaleax commented Jul 27, 2018•
edited
Loading