async_hooks,inspector: inspector keeps async context untouched - #51501
async_hooks,inspector: inspector keeps async context untouched#51501nodejs-github-bot merged 2 commits into
Conversation
Sorry, something went wrong.
|
output of test-inspector-async-context-brk.js before 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.js after 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! |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@nodejs/async_hooks @nodejs/inspector @nodejs/cpp-reviewers |
Sorry, something went wrong.
This comment was marked as outdated.
|
On a side note, the commit message does not adhere to our guidelines and must be amended before this PR can be merged. |
Sorry, something went wrong.
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.
|
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. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Qard
left a comment
There was a problem hiding this comment.
I don't actually see a particular reason why Session 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.
Sorry, something went wrong.
Sorry, something went wrong.
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: nodejs#51501 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
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 <deb2001-github@yahoo.de> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
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 <deb2001-github@yahoo.de> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
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 <deb2001-github@yahoo.de> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
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.