trace_events: 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. PR-URL: https://github.com/nodejs/node/pull/23781 Fixes: https://github.com/nodejs/node/issues/22767 Refs: https://github.com/nodejs/node/issues/22513 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
This commit is contained in:
parent
036fbdb63d
commit
5d80ae3acd
@ -82,6 +82,8 @@ as the one used by `process.hrtime()`
|
|||||||
however the trace-event timestamps are expressed in microseconds,
|
however the trace-event timestamps are expressed in microseconds,
|
||||||
unlike `process.hrtime()` which returns nanoseconds.
|
unlike `process.hrtime()` which returns nanoseconds.
|
||||||
|
|
||||||
|
The features from this module are not available in [`Worker`][] threads.
|
||||||
|
|
||||||
## The `trace_events` module
|
## The `trace_events` module
|
||||||
<!-- YAML
|
<!-- YAML
|
||||||
added: v10.0.0
|
added: v10.0.0
|
||||||
@ -205,3 +207,4 @@ console.log(trace_events.getEnabledCategories());
|
|||||||
[Performance API]: perf_hooks.html
|
[Performance API]: perf_hooks.html
|
||||||
[V8]: v8.html
|
[V8]: v8.html
|
||||||
[`async_hooks`]: async_hooks.html
|
[`async_hooks`]: async_hooks.html
|
||||||
|
[`Worker`]: worker_threads.html#worker_threads_class_worker
|
||||||
|
@ -253,6 +253,7 @@ Notable differences inside a Worker environment are:
|
|||||||
- Execution may stop at any point as a result of [`worker.terminate()`][]
|
- Execution may stop at any point as a result of [`worker.terminate()`][]
|
||||||
being invoked.
|
being invoked.
|
||||||
- IPC channels from parent processes are not accessible.
|
- 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:
|
Currently, the following differences also exist until they are addressed:
|
||||||
|
|
||||||
@ -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
|
[`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
|
||||||
[Signals events]: process.html#process_signal_events
|
[Signals events]: process.html#process_signal_events
|
||||||
[`Uint8Array`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array
|
[`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
|
[browser `MessagePort`]: https://developer.mozilla.org/en-US/docs/Web/API/MessagePort
|
||||||
[child processes]: child_process.html
|
[child processes]: child_process.html
|
||||||
[HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
|
[HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
|
||||||
|
@ -13,7 +13,8 @@ const {
|
|||||||
ERR_INVALID_ARG_TYPE
|
ERR_INVALID_ARG_TYPE
|
||||||
} = require('internal/errors').codes;
|
} = require('internal/errors').codes;
|
||||||
|
|
||||||
if (!hasTracing)
|
const { isMainThread } = require('internal/worker');
|
||||||
|
if (!hasTracing || !isMainThread)
|
||||||
throw new ERR_TRACE_EVENTS_UNAVAILABLE();
|
throw new ERR_TRACE_EVENTS_UNAVAILABLE();
|
||||||
|
|
||||||
const { CategorySet, getEnabledCategories } = internalBinding('trace_events');
|
const { CategorySet, getEnabledCategories } = internalBinding('trace_events');
|
||||||
|
13
src/env.cc
13
src/env.cc
@ -128,11 +128,22 @@ void InitThreadLocalOnce() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
void Environment::TrackingTraceStateObserver::UpdateTraceCategoryState() {
|
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 called 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] =
|
env_->trace_category_state()[0] =
|
||||||
*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(
|
*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(
|
||||||
TRACING_CATEGORY_NODE1(async_hooks));
|
TRACING_CATEGORY_NODE1(async_hooks));
|
||||||
|
|
||||||
Isolate* isolate = env_->isolate();
|
Isolate* isolate = env_->isolate();
|
||||||
|
HandleScope handle_scope(isolate);
|
||||||
Local<Function> cb = env_->trace_category_state_function();
|
Local<Function> cb = env_->trace_category_state_function();
|
||||||
if (cb.IsEmpty())
|
if (cb.IsEmpty())
|
||||||
return;
|
return;
|
||||||
@ -182,7 +193,7 @@ Environment::Environment(IsolateData* isolate_data,
|
|||||||
AssignToContext(context, ContextInfo(""));
|
AssignToContext(context, ContextInfo(""));
|
||||||
|
|
||||||
if (tracing::AgentWriterHandle* writer = GetTracingAgentWriter()) {
|
if (tracing::AgentWriterHandle* writer = GetTracingAgentWriter()) {
|
||||||
trace_state_observer_.reset(new TrackingTraceStateObserver(this));
|
trace_state_observer_ = std::make_unique<TrackingTraceStateObserver>(this);
|
||||||
v8::TracingController* tracing_controller = writer->GetTracingController();
|
v8::TracingController* tracing_controller = writer->GetTracingController();
|
||||||
if (tracing_controller != nullptr)
|
if (tracing_controller != nullptr)
|
||||||
tracing_controller->AddTraceStateObserver(trace_state_observer_.get());
|
tracing_controller->AddTraceStateObserver(trace_state_observer_.get());
|
||||||
|
@ -65,6 +65,10 @@ DispatchResponse TracingAgent::start(
|
|||||||
return DispatchResponse::Error(
|
return DispatchResponse::Error(
|
||||||
"Call NodeTracing::end to stop tracing before updating the config");
|
"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;
|
std::set<std::string> categories_set;
|
||||||
protocol::Array<std::string>* categories =
|
protocol::Array<std::string>* categories =
|
||||||
|
11
test/parallel/test-trace-events-api-worker-disabled.js
Normal file
11
test/parallel/test-trace-events-api-worker-disabled.js
Normal file
@ -0,0 +1,11 @@
|
|||||||
|
// Flags: --experimental-worker
|
||||||
|
'use strict';
|
||||||
|
|
||||||
|
const common = require('../common');
|
||||||
|
const { Worker } = require('worker_threads');
|
||||||
|
|
||||||
|
new Worker("require('trace_events')", { eval: true })
|
||||||
|
.on('error', common.expectsError({
|
||||||
|
code: 'ERR_TRACE_EVENTS_UNAVAILABLE',
|
||||||
|
type: Error
|
||||||
|
}));
|
@ -0,0 +1,28 @@
|
|||||||
|
// Flags: --experimental-worker
|
||||||
|
'use strict';
|
||||||
|
|
||||||
|
const common = require('../common');
|
||||||
|
const { Worker } = require('worker_threads');
|
||||||
|
|
||||||
|
common.skipIfInspectorDisabled();
|
||||||
|
|
||||||
|
if (!process.env.HAS_STARTED_WORKER) {
|
||||||
|
process.env.HAS_STARTED_WORKER = 1;
|
||||||
|
new Worker(__filename);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
const assert = require('assert');
|
||||||
|
const { Session } = require('inspector');
|
||||||
|
|
||||||
|
const session = new Session();
|
||||||
|
session.connect();
|
||||||
|
session.post('NodeTracing.start', {
|
||||||
|
traceConfig: { includedCategories: ['node.perf'] }
|
||||||
|
}, common.mustCall((err) => {
|
||||||
|
assert.deepStrictEqual(err, {
|
||||||
|
code: -32000,
|
||||||
|
message:
|
||||||
|
'Tracing properties can only be changed through main thread sessions'
|
||||||
|
});
|
||||||
|
}));
|
Loading…
x
Reference in New Issue
Block a user