async_hooks,inspector: inspector keeps async context untouched

[dygabo]

This started as an observation that the async context is modified from the function where the breakpoint is defined to the Debugger.paused callback function. The debugger breakpoint handler function should be able to access the async context of the involved function.

I decided for a PR instead of an issue because it allows to make a code proposal to start the discussion. Is there anything that can cause issues with this change?

The testcase with this PR (test-inspector-async-context-brk.js) fails without the change because the Debugger.paused callback runs in the context available at callback creation (in this example undefined). With this modification, the inspector is not considered an async resource and it does not create an own async context and the test passes.

[dygabo]

output of test-inspector-async-context-brk.jsbefore the change:

NOTE: The test started as a child_process using these flags: [ '--expose-internals' ] Use NODE_SKIP_FLAG_CHECK to run the test with the original flags. Connected Debugger was enabled Breakpoint was set on Debugger.paused callback => undefined in code => 1 node:internal/process/promises:289 triggerUncaughtException(err, true /* fromPromise */); ^ AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: 1 !== undefined at test (/home/gabriel/work/github/node/test/parallel/test-inspector-async-context-brk.js:54:10) at process.processTicksAndRejections (node:internal/process/task_queues:95:5){generatedMessage: true, code: 'ERR_ASSERTION', actual: 1, expected: undefined, operator: 'strictEqual' } Node.js v22.0.0-pre 

output of test-inspector-async-context-brk.jsafter the change:

NOTE: The test started as a child_process using these flags: [ '--expose-internals' ] Use NODE_SKIP_FLAG_CHECK to run the test with the original flags. Connected Debugger was enabled Breakpoint was set on Debugger.paused callback => 1 in code => 1 Breakpoint was hit Session disconnected Done! 

[Flarna]

I agree that modeling Inspector as a async resource seems to be wrong as it binds all events and all responses into this single instance.
One might expect that an individual inspector request/response is seen as async resource but I guess this is out of scope for this PR.

[cola119]

@nodejs/async_hooks @nodejs/inspector @nodejs/cpp-reviewers

[tniessen]

On a side note, the commit message does not adhere to our guidelines and must be amended before this PR can be merged.

[dygabo]

thanks @tniessen for the input concerning the commit message. I gave it another try. Please check if the new commit message is compliant. Let me know what is a better suggestion if it is still not matching the requirements.

[dygabo]

I saw in the build from the weekend that there were some failing tests. Are these flaky tests or something that I should be looking at in more detail? @cola119 can you rerun the CI please?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/56885/

[dygabo]

thanks @Flarna for the rerun. It seems to fail differently so I believe retrying it might bring it to be green eventually. I hope we get some reviews to move this forward but there's no rush in that from my point of view. @Qard, @legendecas, @joyeecheung do you have some input on this?

[Flarna]

It seems to fail differently so I believe retrying it might bring it to be green eventually.

yes, will retrigger to prove this once the log statements are gone. Code changes require to rerun CI so I see no need to do it now.

[dygabo]

I updated the test code now but not needed to run the full CI imo. I just was not sure if it's something that needs attention.

[Qard]

I don't actually see a particular reason whySession would need a backing AsyncResource. It's not an async task so much as just another form of event listener, which itself is not async. Event emitters are not themselves async, they just often layer over something else that is async.

In this case the expectation is a sync event triggered whenever the sync code reaches a particular point. This is not a response to a request/task, it is just an observer of what is already happening. It seems to me like this change is reasonable to allow the handler to run in the context of where the breakpoint is reached.

Side note: the original behaviour can also be more easily implemented with events.EventEmitterAsyncResource so it's probably best we do this simplification anyway.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/56904/

[nodejs-github-bot]

Landed in a3abc42