async_hooks,inspector: implement inspector api without async_wrap

Implementing the inspector session object as an async resource causes unwanted context change when a breakpoint callback function is being called. Modelling the inspector api without the AsyncWrap base class ensures that the callback has access to the AsyncLocalStorage instance that is active in the affected user function. See `test-inspector-async-context-brk.js` for an illustration of the use case. PR-URL: #51501 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
nodejs · Feb 15, 2024 · 4ddb9b3 · 4ddb9b3
1 parent 2921d55
commit 4ddb9b3
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 28 deletions.
diff --git a/src/async_wrap.h b/src/async_wrap.h
@@ -102,17 +102,9 @@ namespace node {
 #define NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V)
 #endif  // HAVE_OPENSSL
 
-#if HAVE_INSPECTOR
-#define NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V)                                \
-  V(INSPECTORJSBINDING)
-#else
-#define NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V)
-#endif  // HAVE_INSPECTOR
-
-#define NODE_ASYNC_PROVIDER_TYPES(V)                                          \
-  NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V)                                     \
-  NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V)                                         \
-  NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V)
+#define NODE_ASYNC_PROVIDER_TYPES(V)                                           \
+  NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V)                                      \
+  NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V)
 
 class Environment;
 class DestroyParam;

diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc
@@ -1,4 +1,3 @@
-#include "async_wrap-inl.h"
 #include "base_object-inl.h"
 #include "inspector_agent.h"
 #include "inspector_io.h"
@@ -61,7 +60,7 @@ struct MainThreadConnection {
 };
 
 template <typename ConnectionType>
-class JSBindingsConnection : public AsyncWrap {
+class JSBindingsConnection : public BaseObject {
  public:
   class JSBindingsSessionDelegate : public InspectorSessionDelegate {
    public:
@@ -91,15 +90,16 @@ class JSBindingsConnection : public AsyncWrap {
   JSBindingsConnection(Environment* env,
                        Local<Object> wrap,
                        Local<Function> callback)
-                       : AsyncWrap(env, wrap, PROVIDER_INSPECTORJSBINDING),
-                         callback_(env->isolate(), callback) {
+      : BaseObject(env, wrap), callback_(env->isolate(), callback) {
     Agent* inspector = env->inspector_agent();
     session_ = ConnectionType::Connect(
         inspector, std::make_unique<JSBindingsSessionDelegate>(env, this));
   }
 
   void OnMessage(Local<Value> value) {
-    MakeCallback(callback_.Get(env()->isolate()), 1, &value);
+    auto result = callback_.Get(env()->isolate())
+                      ->Call(env()->context(), object(), 1, &value);
+    (void)result;
   }
 
   static void Bind(Environment* env, Local<Object> target) {
@@ -108,7 +108,6 @@ class JSBindingsConnection : public AsyncWrap {
         NewFunctionTemplate(isolate, JSBindingsConnection::New);
     tmpl->InstanceTemplate()->SetInternalFieldCount(
         JSBindingsConnection::kInternalFieldCount);
-    tmpl->Inherit(AsyncWrap::GetConstructorTemplate(env));
     SetProtoMethod(isolate, tmpl, "dispatch", JSBindingsConnection::Dispatch);
     SetProtoMethod(
         isolate, tmpl, "disconnect", JSBindingsConnection::Disconnect);

diff --git a/test/parallel/test-inspector-async-context-brk.js b/test/parallel/test-inspector-async-context-brk.js
@@ -0,0 +1,56 @@
+'use strict';
+const common = require('../common');
+const { AsyncLocalStorage } = require('async_hooks');
+const als = new AsyncLocalStorage();
+
+function getStore() {
+  return als.getStore();
+}
+
+common.skipIfInspectorDisabled();
+
+const assert = require('assert');
+const { Session } = require('inspector');
+const path = require('path');
+const { pathToFileURL } = require('url');
+
+let valueInFunction = 0;
+let valueInBreakpoint = 0;
+
+function debugged() {
+  valueInFunction = getStore();
+  return 42;
+}
+
+async function test() {
+  const session = new Session();
+
+  session.connect();
+  session.post('Debugger.enable');
+
+  session.on('Debugger.paused', () => {
+    valueInBreakpoint = getStore();
+  });
+
+  await new Promise((resolve, reject) => {
+    session.post('Debugger.setBreakpointByUrl', {
+      'lineNumber': 22,
+      'url': pathToFileURL(path.resolve(__dirname, __filename)).toString(),
+      'columnNumber': 0,
+      'condition': ''
+    }, (error, result) => {
+      return error ? reject(error) : resolve(result);
+    });
+  });
+
+  als.run(1, debugged);
+  assert.strictEqual(valueInFunction, valueInBreakpoint);
+  assert.strictEqual(valueInFunction, 1);
+
+  session.disconnect();
+}
+
+const interval = setInterval(() => {}, 1000);
+test().then(common.mustCall(() => {
+  clearInterval(interval);
+}));
diff --git a/test/parallel/test-inspector-multisession-js.js b/test/parallel/test-inspector-multisession-js.js
@@ -1,4 +1,3 @@
-// Flags: --expose-internals
 'use strict';
 const common = require('../common');
 

diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js
@@ -47,8 +47,6 @@ const { getSystemErrorName } = require('util');
     delete providers.WORKER;
     // TODO(danbev): Test for these
     delete providers.JSUDPWRAP;
-    if (!common.isMainThread)
-      delete providers.INSPECTORJSBINDING;
     delete providers.KEYPAIRGENREQUEST;
     delete providers.KEYGENREQUEST;
     delete providers.KEYEXPORTREQUEST;
@@ -316,13 +314,6 @@ if (common.hasCrypto) { // eslint-disable-line node-core/crypto-check
   testInitialized(req, 'SendWrap');
 }
 
-if (process.features.inspector && common.isMainThread) {
-  const binding = internalBinding('inspector');
-  const handle = new binding.Connection(() => {});
-  testInitialized(handle, 'Connection');
-  handle.disconnect();
-}
-
 // PROVIDER_HEAPDUMP
 {
   v8.getHeapSnapshot().destroy();

diff --git a/typings/internalBinding/async_wrap.d.ts b/typings/internalBinding/async_wrap.d.ts
@@ -68,7 +68,6 @@ declare namespace InternalAsyncWrapBinding {
     SIGNREQUEST: 54;
     TLSWRAP: 55;
     VERIFYREQUEST: 56;
-    INSPECTORJSBINDING: 57;
   }
 }