From 4ddb9b33d5c3d592daf2902088f42e61e1cce93a Mon Sep 17 00:00:00 2001 From: Gabriel Bota <94833492+dygabo@users.noreply.github.com> Date: Wed, 24 Jan 2024 14:29:28 +0100 Subject: [PATCH] async_hooks,inspector: implement inspector api without async_wrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/51501 Reviewed-By: Gerhard Stöbich Reviewed-By: Stephen Belanger --- src/async_wrap.h | 14 +---- src/inspector_js_api.cc | 11 ++-- .../test-inspector-async-context-brk.js | 56 +++++++++++++++++++ .../test-inspector-multisession-js.js | 1 - test/sequential/test-async-wrap-getasyncid.js | 9 --- typings/internalBinding/async_wrap.d.ts | 1 - 6 files changed, 64 insertions(+), 28 deletions(-) create mode 100644 test/parallel/test-inspector-async-context-brk.js diff --git a/src/async_wrap.h b/src/async_wrap.h index 01e981aa671a23..7234d88b67a961 100644 --- 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 index 5c66757afd1a7a..0a2d9e2ec84b08 100644 --- 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 -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 wrap, Local 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(env, this)); } void OnMessage(Local 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 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 new file mode 100644 index 00000000000000..1fd2b45e535966 --- /dev/null +++ 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 index 31aa0c5f569854..81c29e49183645 100644 --- 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 index 7fc7c820d06cc9..cd5957de11e157 100644 --- 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 index a8b583c56967bb..0bcff6e30c6b42 100644 --- 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; } }