From ead1761b8173b262efc36a62c9064915cc38647e Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Wed, 17 Jan 2024 18:59:40 +0100 Subject: [PATCH 1/2] 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. --- src/async_wrap.h | 14 +--- src/inspector_js_api.cc | 11 ++- .../test-inspector-async-context-brk.js | 68 +++++++++++++++++++ .../test-inspector-multisession-js.js | 1 - test/sequential/test-async-wrap-getasyncid.js | 9 --- typings/internalBinding/async_wrap.d.ts | 1 - 6 files changed, 76 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 01e981aa671a..7234d88b67a9 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 5c66757afd1a..0a2d9e2ec84b 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 000000000000..56a40babc017 --- /dev/null +++ b/test/parallel/test-inspector-async-context-brk.js @@ -0,0 +1,68 @@ +'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(); + console.log('in code => ', getStore()); + return 42; +} + +async function test() { + const session = new Session(); + + session.connect(); + console.log('Connected'); + + session.post('Debugger.enable'); + console.log('Debugger was enabled'); + + session.on('Debugger.paused', () => { + valueInBreakpoint = getStore(); + console.log('on Debugger.paused callback => ', 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); + }); + }); + console.log('Breakpoint was set'); + + als.run(1, debugged); + assert.strictEqual(valueInFunction, valueInBreakpoint); + assert.strictEqual(valueInFunction, 1); + assert.notStrictEqual(valueInFunction, 0); + assert.notStrictEqual(valueInBreakpoint, 0); + + console.log('Breakpoint was hit'); + + session.disconnect(); + console.log('Session disconnected'); +} + +const interval = setInterval(() => {}, 1000); +test().then(common.mustCall(() => { + clearInterval(interval); + console.log('Done!'); +})); diff --git a/test/parallel/test-inspector-multisession-js.js b/test/parallel/test-inspector-multisession-js.js index 31aa0c5f5698..81c29e491836 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 7fc7c820d06c..cd5957de11e1 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 a8b583c56967..0bcff6e30c6b 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; } } From bf08b03d32db80d3f5518a89fc4e94c1d7634825 Mon Sep 17 00:00:00 2001 From: Gabriel Bota Date: Tue, 23 Jan 2024 11:16:50 +0100 Subject: [PATCH 2/2] fixup! async_hooks,inspector: implement inspector api without async_wrap --- test/parallel/test-inspector-async-context-brk.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/test/parallel/test-inspector-async-context-brk.js b/test/parallel/test-inspector-async-context-brk.js index 56a40babc017..1fd2b45e5359 100644 --- a/test/parallel/test-inspector-async-context-brk.js +++ b/test/parallel/test-inspector-async-context-brk.js @@ -19,7 +19,6 @@ let valueInBreakpoint = 0; function debugged() { valueInFunction = getStore(); - console.log('in code => ', getStore()); return 42; } @@ -27,14 +26,10 @@ async function test() { const session = new Session(); session.connect(); - console.log('Connected'); - session.post('Debugger.enable'); - console.log('Debugger was enabled'); session.on('Debugger.paused', () => { valueInBreakpoint = getStore(); - console.log('on Debugger.paused callback => ', getStore()); }); await new Promise((resolve, reject) => { @@ -47,22 +42,15 @@ async function test() { return error ? reject(error) : resolve(result); }); }); - console.log('Breakpoint was set'); als.run(1, debugged); assert.strictEqual(valueInFunction, valueInBreakpoint); assert.strictEqual(valueInFunction, 1); - assert.notStrictEqual(valueInFunction, 0); - assert.notStrictEqual(valueInBreakpoint, 0); - - console.log('Breakpoint was hit'); session.disconnect(); - console.log('Session disconnected'); } const interval = setInterval(() => {}, 1000); test().then(common.mustCall(() => { clearInterval(interval); - console.log('Done!'); }));