From f9ddbb6b2f2ff61bb62a4ee8a8819d561322c9e8 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 9 Mar 2019 09:28:04 +0100 Subject: [PATCH] lib: move DTRACE_* probes out of global scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DTRACE_* probes have been global for no really good reason. Move those into an internalBinding. PR-URL: https://github.com/nodejs/node/pull/26541 Reviewed-By: Anna Henningsen Reviewed-By: Michaël Zasso Reviewed-By: Matteo Collina Reviewed-By: Matheus Marchini Reviewed-By: Gus Caplan Reviewed-By: Ruben Bridgewater --- .eslintrc.js | 6 ---- lib/_http_client.js | 4 +++ lib/_http_server.js | 4 +++ lib/internal/dtrace.js | 21 ++++++++++++ lib/net.js | 4 +++ node.gyp | 1 + src/node.cc | 2 +- src/node_binding.cc | 9 +++++- src/node_config.cc | 4 +++ src/node_dtrace.cc | 46 ++++++++++++--------------- src/node_dtrace.h | 2 +- test/common/index.js | 9 ------ test/parallel/test-global.js | 10 ------ test/parallel/test-internal-dtrace.js | 24 ++++++++++++++ 14 files changed, 92 insertions(+), 54 deletions(-) create mode 100644 lib/internal/dtrace.js create mode 100644 test/parallel/test-internal-dtrace.js diff --git a/.eslintrc.js b/.eslintrc.js index 475c75ff962dac..23b707b55d9062 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -281,12 +281,6 @@ module.exports = { BigInt: 'readable', BigInt64Array: 'readable', BigUint64Array: 'readable', - DTRACE_HTTP_CLIENT_REQUEST: 'readable', - DTRACE_HTTP_CLIENT_RESPONSE: 'readable', - DTRACE_HTTP_SERVER_REQUEST: 'readable', - DTRACE_HTTP_SERVER_RESPONSE: 'readable', - DTRACE_NET_SERVER_CONNECTION: 'readable', - DTRACE_NET_STREAM_END: 'readable', TextEncoder: 'readable', TextDecoder: 'readable', queueMicrotask: 'readable', diff --git a/lib/_http_client.js b/lib/_http_client.js index de7b01cbd1b9bd..4e5912608425b9 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -47,6 +47,10 @@ const { } = require('internal/errors').codes; const { validateTimerDuration } = require('internal/timers'); const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol; +const { + DTRACE_HTTP_CLIENT_REQUEST, + DTRACE_HTTP_CLIENT_RESPONSE +} = require('internal/dtrace'); const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; diff --git a/lib/_http_server.js b/lib/_http_server.js index 95f08fa5d6ace9..5941e5740fcf91 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -50,6 +50,10 @@ const { ERR_INVALID_CHAR } = require('internal/errors').codes; const Buffer = require('buffer').Buffer; +const { + DTRACE_HTTP_SERVER_REQUEST, + DTRACE_HTTP_SERVER_RESPONSE +} = require('internal/dtrace'); const kServerResponse = Symbol('ServerResponse'); diff --git a/lib/internal/dtrace.js b/lib/internal/dtrace.js new file mode 100644 index 00000000000000..8eb1df7807fbcd --- /dev/null +++ b/lib/internal/dtrace.js @@ -0,0 +1,21 @@ +'use strict'; + +const config = internalBinding('config'); + +const { + DTRACE_HTTP_CLIENT_REQUEST = () => {}, + DTRACE_HTTP_CLIENT_RESPONSE = () => {}, + DTRACE_HTTP_SERVER_REQUEST = () => {}, + DTRACE_HTTP_SERVER_RESPONSE = () => {}, + DTRACE_NET_SERVER_CONNECTION = () => {}, + DTRACE_NET_STREAM_END = () => {} +} = (config.hasDtrace ? internalBinding('dtrace') : {}); + +module.exports = { + DTRACE_HTTP_CLIENT_REQUEST, + DTRACE_HTTP_CLIENT_RESPONSE, + DTRACE_HTTP_SERVER_REQUEST, + DTRACE_HTTP_SERVER_RESPONSE, + DTRACE_NET_SERVER_CONNECTION, + DTRACE_NET_STREAM_END +}; diff --git a/lib/net.js b/lib/net.js index 8d69cbaf813051..0e68e954002a23 100644 --- a/lib/net.js +++ b/lib/net.js @@ -85,6 +85,10 @@ const { } = require('internal/errors'); const { validateInt32, validateString } = require('internal/validators'); const kLastWriteQueueSize = Symbol('lastWriteQueueSize'); +const { + DTRACE_NET_SERVER_CONNECTION, + DTRACE_NET_STREAM_END +} = require('internal/dtrace'); // Lazy loaded to improve startup performance. let cluster; diff --git a/node.gyp b/node.gyp index f5b09f5ffae941..1c7484378fbf5c 100644 --- a/node.gyp +++ b/node.gyp @@ -116,6 +116,7 @@ 'lib/internal/dns/promises.js', 'lib/internal/dns/utils.js', 'lib/internal/domexception.js', + 'lib/internal/dtrace.js', 'lib/internal/encoding.js', 'lib/internal/errors.js', 'lib/internal/error-serdes.js', diff --git a/src/node.cc b/src/node.cc index 5e0e8df4214450..adcd6f6cadb55d 100644 --- a/src/node.cc +++ b/src/node.cc @@ -255,7 +255,7 @@ MaybeLocal RunBootstrapping(Environment* env) { Local global = context->Global(); #if defined HAVE_DTRACE || defined HAVE_ETW - InitDTrace(env, global); + InitDTrace(env); #endif Local process = env->process_object(); diff --git a/src/node_binding.cc b/src/node_binding.cc index 9599cf956b9b7c..123bdad418e446 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -28,6 +28,12 @@ #define NODE_BUILTIN_PROFILER_MODULES(V) #endif +#if HAVE_DTRACE || HAVE_ETW +#define NODE_BUILTIN_DTRACE_MODULES(V) V(dtrace) +#else +#define NODE_BUILTIN_DTRACE_MODULES(V) +#endif + // A list of built-in modules. In order to do module registration // in node::Init(), need to add built-in modules in the following list. // Then in binding::RegisterBuiltinModules(), it calls modules' registration @@ -85,7 +91,8 @@ NODE_BUILTIN_OPENSSL_MODULES(V) \ NODE_BUILTIN_ICU_MODULES(V) \ NODE_BUILTIN_REPORT_MODULES(V) \ - NODE_BUILTIN_PROFILER_MODULES(V) + NODE_BUILTIN_PROFILER_MODULES(V) \ + NODE_BUILTIN_DTRACE_MODULES(V) // This is used to load built-in modules. Instead of using // __attribute__((constructor)), we call the _register_ diff --git a/src/node_config.cc b/src/node_config.cc index a0d7d25ec86634..c3c962bd8869af 100644 --- a/src/node_config.cc +++ b/src/node_config.cc @@ -74,6 +74,10 @@ static void Initialize(Local target, READONLY_PROPERTY(target, "bits", Number::New(env->isolate(), 8 * sizeof(intptr_t))); + +#if defined HAVE_DTRACE || defined HAVE_ETW + READONLY_TRUE_PROPERTY(target, "hasDtrace"); +#endif } // InitConfig } // namespace node diff --git a/src/node_dtrace.cc b/src/node_dtrace.cc index 59d1f9d143ac1d..5b2b9b8e14e942 100644 --- a/src/node_dtrace.cc +++ b/src/node_dtrace.cc @@ -48,6 +48,7 @@ namespace node { +using v8::Context; using v8::FunctionCallbackInfo; using v8::GCCallbackFlags; using v8::GCType; @@ -262,31 +263,7 @@ void dtrace_gc_done(Isolate* isolate, GCType type, GCCallbackFlags flags) { } -void InitDTrace(Environment* env, Local target) { - HandleScope scope(env->isolate()); - - static struct { - const char* name; - void (*func)(const FunctionCallbackInfo&); - } tab[] = { -#define NODE_PROBE(name) #name, name - { NODE_PROBE(DTRACE_NET_SERVER_CONNECTION) }, - { NODE_PROBE(DTRACE_NET_STREAM_END) }, - { NODE_PROBE(DTRACE_HTTP_SERVER_REQUEST) }, - { NODE_PROBE(DTRACE_HTTP_SERVER_RESPONSE) }, - { NODE_PROBE(DTRACE_HTTP_CLIENT_REQUEST) }, - { NODE_PROBE(DTRACE_HTTP_CLIENT_RESPONSE) } -#undef NODE_PROBE - }; - - for (size_t i = 0; i < arraysize(tab); i++) { - Local key = OneByteString(env->isolate(), tab[i].name); - Local val = env->NewFunctionTemplate(tab[i].func) - ->GetFunction(env->context()) - .ToLocalChecked(); - target->Set(env->context(), key, val).FromJust(); - } - +void InitDTrace(Environment* env) { #ifdef HAVE_ETW // ETW is neither thread-safe nor does it clean up resources on exit, // so we can use it only on the main thread. @@ -295,10 +272,27 @@ void InitDTrace(Environment* env, Local target) { } #endif -#if defined HAVE_DTRACE || defined HAVE_ETW env->isolate()->AddGCPrologueCallback(dtrace_gc_start); env->isolate()->AddGCEpilogueCallback(dtrace_gc_done); +} + +void InitializeDTrace(Local target, + Local unused, + Local context, + void* priv) { + Environment* env = Environment::GetCurrent(context); + +#if defined HAVE_DTRACE || defined HAVE_ETW +# define NODE_PROBE(name) env->SetMethod(target, #name, name); + NODE_PROBE(DTRACE_NET_SERVER_CONNECTION) + NODE_PROBE(DTRACE_NET_STREAM_END) + NODE_PROBE(DTRACE_HTTP_SERVER_REQUEST) + NODE_PROBE(DTRACE_HTTP_SERVER_RESPONSE) + NODE_PROBE(DTRACE_HTTP_CLIENT_REQUEST) + NODE_PROBE(DTRACE_HTTP_CLIENT_RESPONSE) +# undef NODE_PROBE #endif } } // namespace node +NODE_MODULE_CONTEXT_AWARE_INTERNAL(dtrace, node::InitializeDTrace) diff --git a/src/node_dtrace.h b/src/node_dtrace.h index ee11cd68aa7630..cbabe905597cb0 100644 --- a/src/node_dtrace.h +++ b/src/node_dtrace.h @@ -76,7 +76,7 @@ typedef struct { namespace node { -void InitDTrace(Environment* env, v8::Local target); +void InitDTrace(Environment* env); } // namespace node diff --git a/test/common/index.js b/test/common/index.js index ac983d0a85a4dd..94b90ac27ba034 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -266,15 +266,6 @@ if (global.gc) { knownGlobals.push(global.gc); } -if (global.DTRACE_HTTP_SERVER_RESPONSE) { - knownGlobals.push(DTRACE_HTTP_SERVER_RESPONSE); - knownGlobals.push(DTRACE_HTTP_SERVER_REQUEST); - knownGlobals.push(DTRACE_HTTP_CLIENT_RESPONSE); - knownGlobals.push(DTRACE_HTTP_CLIENT_REQUEST); - knownGlobals.push(DTRACE_NET_STREAM_END); - knownGlobals.push(DTRACE_NET_SERVER_CONNECTION); -} - if (process.env.NODE_TEST_KNOWN_GLOBALS) { const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(','); allowGlobals(...knownFromEnv); diff --git a/test/parallel/test-global.js b/test/parallel/test-global.js index dd3f37ee93a3d7..ff34a812c592da 100644 --- a/test/parallel/test-global.js +++ b/test/parallel/test-global.js @@ -51,16 +51,6 @@ builtinModules.forEach((moduleName) => { 'setInterval', 'setTimeout' ]; - if (global.DTRACE_HTTP_SERVER_RESPONSE) { - expected.unshift( - 'DTRACE_HTTP_SERVER_RESPONSE', - 'DTRACE_HTTP_SERVER_REQUEST', - 'DTRACE_HTTP_CLIENT_RESPONSE', - 'DTRACE_HTTP_CLIENT_REQUEST', - 'DTRACE_NET_STREAM_END', - 'DTRACE_NET_SERVER_CONNECTION' - ); - } assert.deepStrictEqual(new Set(Object.keys(global)), new Set(expected)); } diff --git a/test/parallel/test-internal-dtrace.js b/test/parallel/test-internal-dtrace.js new file mode 100644 index 00000000000000..68991d739a8bf4 --- /dev/null +++ b/test/parallel/test-internal-dtrace.js @@ -0,0 +1,24 @@ +// Flags: --expose-internals +'use strict'; + +require('../common'); +const assert = require('assert'); + +const { + DTRACE_HTTP_CLIENT_REQUEST, + DTRACE_HTTP_CLIENT_RESPONSE, + DTRACE_HTTP_SERVER_REQUEST, + DTRACE_HTTP_SERVER_RESPONSE, + DTRACE_NET_SERVER_CONNECTION, + DTRACE_NET_STREAM_END +} = require('internal/dtrace'); + +// We're just testing to make sure these are always defined and +// callable. We don't actually test their function here. + +assert.strictEqual(typeof DTRACE_HTTP_CLIENT_REQUEST, 'function'); +assert.strictEqual(typeof DTRACE_HTTP_CLIENT_RESPONSE, 'function'); +assert.strictEqual(typeof DTRACE_HTTP_SERVER_REQUEST, 'function'); +assert.strictEqual(typeof DTRACE_HTTP_SERVER_RESPONSE, 'function'); +assert.strictEqual(typeof DTRACE_NET_SERVER_CONNECTION, 'function'); +assert.strictEqual(typeof DTRACE_NET_STREAM_END, 'function');