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]>

src/async_wrap.h

@@ -98,17 +98,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;

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);

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);
+}));

test/parallel/test-inspector-multisession-js.js

@@ -1,4 +1,3 @@
-// Flags: --expose-internals
 'use strict';
 const common = require('../common');
 

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;
@@ -312,13 +310,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();

typings/internalBinding/async_wrap.d.ts

@@ -68,7 +68,6 @@ declare namespace InternalAsyncWrapBinding {
     SIGNREQUEST: 54;
     TLSWRAP: 55;
     VERIFYREQUEST: 56;
-    INSPECTORJSBINDING: 57;
   }
 }