Skip to content

Commit

Permalink
tracing: forbid tracing modifications from worker threads
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
addaleax committed Oct 21, 2018
1 parent afef60e commit 443bde0
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 1 deletion.
2 changes: 2 additions & 0 deletions doc/api/worker_threads.md
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ Notable differences inside a Worker environment are:
- Execution may stop at any point as a result of [`worker.terminate()`][]
being invoked.
- IPC channels from parent processes are not accessible.
- The [`trace_events`][] module is not supported.

Currently, the following differences also exist until they are addressed:

Expand Down Expand Up @@ -489,6 +490,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
[`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
[Signals events]: process.html#process_signal_events
[`Uint8Array`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array
[`trace_events`]: tracing.html
[browser `MessagePort`]: https://developer.mozilla.org/en-US/docs/Web/API/MessagePort
[child processes]: child_process.html
[HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
Expand Down
3 changes: 2 additions & 1 deletion lib/trace_events.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ const {
ERR_INVALID_ARG_TYPE
} = require('internal/errors').codes;

if (!hasTracing)
const { isMainThread } = require('internal/worker');
if (!hasTracing || !isMainThread)
throw new ERR_TRACE_EVENTS_UNAVAILABLE();

const { CategorySet, getEnabledCategories } = internalBinding('trace_events');
Expand Down
11 changes: 11 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,22 @@ void InitThreadLocalOnce() {
}

void Environment::TrackingTraceStateObserver::UpdateTraceCategoryState() {
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
// `StopTracing()`. The only way to do this in a threadsafe fashion
// seems to be only tracking this from the main thread, and only allowing
// these state modifications from the main thread.
return;
}

env_->trace_category_state()[0] =
*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(
TRACING_CATEGORY_NODE1(async_hooks));

Isolate* isolate = env_->isolate();
HandleScope handle_scope(isolate);
Local<Function> cb = env_->trace_category_state_function();
if (cb.IsEmpty())
return;
Expand Down
4 changes: 4 additions & 0 deletions src/inspector/tracing_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ DispatchResponse TracingAgent::start(
return DispatchResponse::Error(
"Call NodeTracing::end to stop tracing before updating the config");
}
if (!env_->is_main_thread()) {
return DispatchResponse::Error(
"Tracing properties can only be changed through main thread sessions");
}

std::set<std::string> categories_set;
protocol::Array<std::string>* categories =
Expand Down

0 comments on commit 443bde0

Please sign in to comment.