lib: move DTRACE_* probes out of global scope

The DTRACE_* probes have been global for no really good reason. Move those into an internalBinding. PR-URL: #26541 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
nodejs · Mar 12, 2019 · f9ddbb6 · f9ddbb6
1 parent f2064df
commit f9ddbb6
Show file tree

Hide file tree

Showing 14 changed files with 92 additions and 54 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -255,7 +255,7 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
   Local<Object> global = context->Global();
 
 #if defined HAVE_DTRACE || defined HAVE_ETW
-  InitDTrace(env, global);
+  InitDTrace(env);
 #endif
 
   Local<Object> process = env->process_object();

diff --git 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_<modname>

diff --git a/src/node_config.cc b/src/node_config.cc
@@ -74,6 +74,10 @@ static void Initialize(Local<Object> 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
@@ -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<Object> target) {
-  HandleScope scope(env->isolate());
-
-  static struct {
-    const char* name;
-    void (*func)(const FunctionCallbackInfo<Value>&);
-  } 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<String> key = OneByteString(env->isolate(), tab[i].name);
-    Local<Value> 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<Object> target) {
   }
 #endif
 
-#if defined HAVE_DTRACE || defined HAVE_ETW
   env->isolate()->AddGCPrologueCallback(dtrace_gc_start);
   env->isolate()->AddGCEpilogueCallback(dtrace_gc_done);
+}
+
+void InitializeDTrace(Local<Object> target,
+                      Local<Value> unused,
+                      Local<Context> 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
@@ -76,7 +76,7 @@ typedef struct {
 
 namespace node {
 
-void InitDTrace(Environment* env, v8::Local<v8::Object> target);
+void InitDTrace(Environment* env);
 
 }  // namespace node
 

diff --git 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
@@ -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
@@ -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');