Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks,inspector: inspector keeps async context untouched #51501

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

dygabo
Copy link
Member

@dygabo dygabo commented Jan 17, 2024

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.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 17, 2024
@dygabo
Copy link
Member Author

dygabo commented Jan 17, 2024

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!

@dygabo dygabo force-pushed the inspector_not_async_resource branch 2 times, most recently from 29935b8 to dfedbf9 Compare January 17, 2024 17:59
@Flarna Flarna added inspector Issues and PRs related to the V8 inspector protocol async_hooks Issues and PRs related to the async hooks subsystem. labels Jan 17, 2024
@Flarna
Copy link
Member

Flarna commented Jan 18, 2024

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
Copy link
Member

cola119 commented Jan 21, 2024

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

@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2024
@nodejs-github-bot

This comment was marked as outdated.

@tniessen
Copy link
Member

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

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.
@dygabo dygabo force-pushed the inspector_not_async_resource branch from 259b0ef to ead1761 Compare January 21, 2024 21:23
@dygabo
Copy link
Member Author

dygabo commented Jan 21, 2024

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
Copy link
Member Author

dygabo commented Jan 22, 2024

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?

@Flarna Flarna added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 22, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2024
@nodejs-github-bot
Copy link
Collaborator

@dygabo
Copy link
Member Author

dygabo commented Jan 23, 2024

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
Copy link
Member

Flarna commented Jan 23, 2024

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
Copy link
Member Author

dygabo commented Jan 23, 2024

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.

@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2024
@Flarna Flarna added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@Qard Qard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nodejs-github-bot
Copy link
Collaborator

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

@Flarna Flarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2024
@nodejs-github-bot nodejs-github-bot merged commit a3abc42 into nodejs:main Jan 24, 2024
57 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a3abc42

@dygabo dygabo deleted the inspector_not_async_resource branch January 24, 2024 16:33
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 2, 2024
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>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Feb 9, 2024
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>
targos pushed a commit that referenced this pull request Feb 15, 2024
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>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 19, 2024
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>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
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>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
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>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants