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 ofrobots, eugeneo and jasnell 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 0bbe35a Compare 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

renovate bot mentioned this pull request Feb 8, 2019

Update Node.js to v10.9.0 mozilla/treeherder#3929

Merged

renovate bot 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

src: refactor tracing code #21867

src: refactor tracing code #21867

addaleax commented Jul 18, 2018

nodejs-github-bot commented Jul 18, 2018

addaleax commented Jul 18, 2018

eugeneo commented Jul 18, 2018

addaleax commented Jul 18, 2018

eugeneo commented Jul 19, 2018

eugeneo commented Jul 23, 2018

ofrobots commented Jul 26, 2018

addaleax commented Jul 27, 2018 •

edited

Loading

src: refactor tracing code #21867

src: refactor tracing code #21867

Conversation

addaleax commented Jul 18, 2018

Checklist

nodejs-github-bot commented Jul 18, 2018

addaleax commented Jul 18, 2018

eugeneo commented Jul 18, 2018

addaleax commented Jul 18, 2018

eugeneo commented Jul 19, 2018

eugeneo commented Jul 23, 2018

ofrobots commented Jul 26, 2018

addaleax commented Jul 27, 2018 • edited Loading

addaleax commented Jul 27, 2018 •

edited

Loading