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.

Sign up for GitHub

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

Jump to bottom

inspector: patch C++ debug options instead of process._breakFirstLine #26602

Closed
wants to merge 2 commits into from
Closed

inspector: patch C++ debug options instead of process._breakFirstLine #26602

wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

Instead of patching process._breakFirstLine to inform the JS land
to wait for the debugger, check that the JS land has not yet
serialized the options and then patch the debug options from C++.
The changes will be carried into JS later during option serialization.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@joyeecheung
inspector: patch C++ debug options instead of process._breakFirstLine
ed6b456 
Instead of patching process._breakFirstLine to inform the JS land
to wait for the debugger, check that the JS land has not yet
serialized the options and then patch the debug options from C++.
The changes will be carried into JS later during option serialization.
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2875/pipeline

@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. labels Mar 12, 2019
@joyeecheung
Copy link
Member Author

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

@addaleax
Copy link
Member

This seems like something that should be handled during options parsing, tbh, unless there’s any reason not to do that?

I’m not really a fan of adding the has_serialized_options() state here…

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 12, 2019 •
edited
Loading

@addaleax The states here are determined when the NodeWorker.enable message is dispatched on the main thread with waitForDebuggerOnStart = true, and not through option parsing, I don't think there is a way to handle this in the parser? We can only determine this value when we start the inspector agent of the worker by querying the parent, it's not ideal that we have to update the option values like this, but I guess it's at least better than updating process._breakFirstLine in the worker...for one, that result in inconsistent states, since the source of truth is in the C++ options, but only process._breakFirstLine is updated, so we even get getOptionValue('--inspect-brk') !== process._breakFirstLine in the JS land of the worker.

has_serialized_options here is a safety net to check that we don't run into weird races - that is, the child worker's inspector agent should not be started after or during pre-execution of the worker (that's probably impossible at the moment, since StartWorkerInspector is invoked before RunBootstrapping, so it's just guarding against future bugs).

@joyeecheung
Copy link
Member Author

hmm..I guess we could also normalize the child's debug options in Worker::New by querying the state of env->inspector_agent() eagerly at that point, not sure if that pass the test yet..

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 12, 2019 •
edited
Loading

Well, looks like we can't patch the options in Worker::New after all, because if the main thread dispatch a NodeWorker.disable in a bad time and we patch the options too early, there would be a race and fail the test...

jdalton
jdalton reviewed Mar 12, 2019
View reviewed changes
src/env.h Show resolved Hide resolved
@addaleax
Copy link
Member

has_serialized_options here is a safety net to check that we don't run into weird races - that is, the child worker's inspector agent should not be started after or during pre-execution of the worker (that's probably impossible at the moment, since StartWorkerInspector is invoked before RunBootstrapping, so it's just guarding against future bugs).

@joyeecheung Can we rather check that then? That the inspector agent is not started during/after pre-execution? I’d like to be able to call getOptions() multiple times in the future…

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 12, 2019 •
edited
Loading

@addaleax I could remove the check in GetOptions to allow multiple serializations, Agent::Start just needs to make sure that GetOptions has not been called before, it does not really matter how many times it is called afterwards.

@joyeecheung
Copy link
Member Author

Removed the check in GetOptions to allow multiple serializations.

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2019
@joyeecheung
Copy link
Member Author

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

@joyeecheung
Copy link
Member Author

Resume: https://ci.nodejs.org/job/node-test-pull-request/21642/

@joyeecheung
Copy link
Member Author

Landed in 97a919b

joyeecheung added a commit that referenced this pull request Mar 18, 2019
@joyeecheung
inspector: patch C++ debug options instead of process._breakFirstLine
97a919b 
Instead of patching process._breakFirstLine to inform the JS land
to wait for the debugger, check that the JS land has not yet
serialized the options and then patch the debug options from C++.
The changes will be carried into JS later during option serialization.

PR-URL: #26602
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Mar 22, 2019
Closed
targos pushed a commit that referenced this pull request Mar 28, 2019
@joyeecheung @targos
inspector: patch C++ debug options instead of process._breakFirstLine
423f363 
Instead of patching process._breakFirstLine to inform the JS land
to wait for the debugger, check that the JS land has not yet
serialized the options and then patch the debug options from C++.
The changes will be carried into JS later during option serialization.

PR-URL: #26602
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Mar 30, 2019
@joyeecheung @targos
inspector: patch C++ debug options instead of process._breakFirstLine
1b45704 
Instead of patching process._breakFirstLine to inform the JS land
to wait for the debugger, check that the JS land has not yet
serialized the options and then patch the debug options from C++.
The changes will be carried into JS later during option serialization.

PR-URL: #26602
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdalton jdalton jdalton left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax addaleax approved these changes

@eugeneo eugeneo eugeneo approved these changes

Assignees
No one assigned
Labels
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joyeecheung @nodejs-github-bot @addaleax @jdalton @eugeneo @BridgeAR