tracing: refactor tracing state ownership #23781

addaleax · 2018-10-20T10:00:04Z

src: remove Environment::tracing_agent_writer()

As per the conversation in Scope of inspector/tracing agents #22513,
this is essentially global, and adding this on the Environment
is generally just confusing.

Refs: Scope of inspector/tracing agents #22513
Fixes: node::Environment::tracing_agent_writer returns nullptr in a worker #22767
tracing: forbid tracing modifications from worker threads

Forbid modifying tracing state from worker threads, either
through the built-in module or inspector sessions, since
the main thread owns all global state, and at least
the async_hooks integration is definitely not thread
safe in its current state.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-10-20T10:00:05Z

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

richardlau · 2018-10-20T13:00:56Z

Does the second commit need tests?

jasnell · 2018-10-20T13:24:10Z

lib/trace_events.js

@@ -13,7 +13,8 @@ const {
  ERR_INVALID_ARG_TYPE
 } = require('internal/errors').codes;

-if (!hasTracing)
+const { isMainThread } = require('internal/worker');
+if (!hasTracing || !isMainThread)


A note about this limitation should be added in tracing.md also

refack

Left some comments and questions.

refack · 2018-10-20T13:44:06Z

src/env.cc

+  if (!env_->is_main_thread()) {
+    // Ideally, we’d have a consistent story that treats all threads/Environment
+    // instances equally here. However, tracing is essentially global, and this
+    // callback is callback from whichever thread calls `StartTracing()` or


Suggested change

// callback is callback from whichever thread calls `StartTracing()` or

// callback is called from whichever thread calls `StartTracing()` or

or invoked or called back

refack · 2018-10-20T13:54:54Z

src/env.cc

@@ -182,10 +191,9 @@ Environment::Environment(IsolateData* isolate_data,

  AssignToContext(context, ContextInfo(""));

-  if (tracing_agent_writer_ != nullptr) {
+  if (tracing::AgentWriterHandle* writer = GetTracingAgentWriter()) {
    trace_state_observer_.reset(new TrackingTraceStateObserver(this));


Suggested change

trace_state_observer_.reset(new TrackingTraceStateObserver(this));

trace_state_observer_ = std::make_unique<TrackingTraceStateObserver>(this);

src/env.cc

src/inspector/tracing_agent.cc

src/node_worker.cc

src/node.cc

src/node_internals.h

As per the conversation in nodejs#22513, this is essentially global, and adding this on the Environment is generally just confusing. Refs: nodejs#22513 Fixes: nodejs#22767

Forbid modifying tracing state from worker threads, either through the built-in module or inspector sessions, since the main thread owns all global state, and at least the `async_hooks` integration is definitely not thread safe in its current state.

addaleax · 2018-10-21T09:47:16Z

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

@richardlau @jasnell PTAL

addaleax · 2018-10-24T10:22:45Z

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18103/

mmarchini · 2018-10-24T14:22:35Z

Landed in d568b53...5d80ae3

As per the conversation in #22513, this is essentially global, and adding this on the Environment is generally just confusing. Refs: #22513 Fixes: #22767 PR-URL: #23781 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

Forbid modifying tracing state from worker threads, either through the built-in module or inspector sessions, since the main thread owns all global state, and at least the `async_hooks` integration is definitely not thread safe in its current state. PR-URL: #23781 Fixes: #22767 Refs: #22513 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

As per the conversation in #22513, this is essentially global, and adding this on the Environment is generally just confusing. Refs: #22513 Fixes: #22767 PR-URL: #23781 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

Forbid modifying tracing state from worker threads, either through the built-in module or inspector sessions, since the main thread owns all global state, and at least the `async_hooks` integration is definitely not thread safe in its current state. PR-URL: #23781 Fixes: #22767 Refs: #22513 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

targos · 2018-11-01T20:44:14Z

Marking dont-land-on-v11.x until nodejs/build#1542 is fixed

As per the conversation in #22513, this is essentially global, and adding this on the Environment is generally just confusing. Refs: #22513 Fixes: #22767 PR-URL: #23781 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

Forbid modifying tracing state from worker threads, either through the built-in module or inspector sessions, since the main thread owns all global state, and at least the `async_hooks` integration is definitely not thread safe in its current state. PR-URL: #23781 Fixes: #22767 Refs: #22513 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

As per the conversation in #22513, this is essentially global, and adding this on the Environment is generally just confusing. Refs: #22513 Fixes: #22767 PR-URL: #23781 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

Forbid modifying tracing state from worker threads, either through the built-in module or inspector sessions, since the main thread owns all global state, and at least the `async_hooks` integration is definitely not thread safe in its current state. PR-URL: #23781 Fixes: #22767 Refs: #22513 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

addaleax requested review from eugeneo and jasnell October 20, 2018 10:00

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 Oct 20, 2018

jasnell reviewed Oct 20, 2018

View reviewed changes

refack reviewed Oct 20, 2018

View reviewed changes

addaleax added 3 commits October 21, 2018 03:25

src: remove Environment::tracing_agent_writer()

afef60e

As per the conversation in nodejs#22513, this is essentially global, and adding this on the Environment is generally just confusing. Refs: nodejs#22513 Fixes: nodejs#22767

squash! tracing: forbid tracing modifications from worker threads

66094c3

addaleax force-pushed the env-tracing-writer branch from cecac2b to 66094c3 Compare October 21, 2018 01:43

refack approved these changes Oct 21, 2018

View reviewed changes

richardlau approved these changes Oct 21, 2018

View reviewed changes

mmarchini approved these changes Oct 23, 2018

View reviewed changes

mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 23, 2018

mmarchini closed this Oct 24, 2018

addaleax deleted the env-tracing-writer branch October 24, 2018 17:56

targos added backport-requested-v11.x and removed backport-requested-v11.x labels Oct 27, 2018

targos mentioned this pull request Oct 27, 2018

Release proposal: v11.1.0 #23922

Merged

targos added the dont-land-on-v11.x label Nov 1, 2018

targos mentioned this pull request Nov 1, 2018

src: fix compiler warnings #23880

Closed

2 tasks

targos removed the dont-land-on-v11.x label Nov 18, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

codebytere added the dont-land-on-v10.x label Jan 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: refactor tracing state ownership #23781

tracing: refactor tracing state ownership #23781

addaleax commented Oct 20, 2018

nodejs-github-bot commented Oct 20, 2018

richardlau commented Oct 20, 2018

jasnell Oct 20, 2018

addaleax Oct 21, 2018

refack left a comment

refack Oct 20, 2018

addaleax Oct 21, 2018

refack Oct 20, 2018

addaleax Oct 21, 2018

addaleax commented Oct 21, 2018

addaleax commented Oct 24, 2018

mmarchini commented Oct 24, 2018

targos commented Nov 1, 2018

	// callback is callback from whichever thread calls `StartTracing()` or
	// callback is called from whichever thread calls `StartTracing()` or

	trace_state_observer_.reset(new TrackingTraceStateObserver(this));
	trace_state_observer_ = std::make_unique<TrackingTraceStateObserver>(this);

tracing: refactor tracing state ownership #23781

tracing: refactor tracing state ownership #23781

Conversation

addaleax commented Oct 20, 2018

Checklist

nodejs-github-bot commented Oct 20, 2018

richardlau commented Oct 20, 2018

jasnell Oct 20, 2018

Choose a reason for hiding this comment

addaleax Oct 21, 2018

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

refack Oct 20, 2018

Choose a reason for hiding this comment

addaleax Oct 21, 2018

Choose a reason for hiding this comment

refack Oct 20, 2018

Choose a reason for hiding this comment

addaleax Oct 21, 2018

Choose a reason for hiding this comment

addaleax commented Oct 21, 2018

addaleax commented Oct 24, 2018

mmarchini commented Oct 24, 2018

targos commented Nov 1, 2018