Skip to content

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

Closed
joyeecheung wants to merge 2 commits intonodejs:masterfrom
joyeecheung:patch-brk
Closed

inspector: patch C++ debug options instead of process._breakFirstLine#26602
joyeecheung wants to merge 2 commits intonodejs:masterfrom
joyeecheung:patch-brk

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 12, 2019

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

nodejs-github-bot commented Mar 12, 2019

@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

joyeecheung commented Mar 12, 2019

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

@addaleax
Copy link
Member

addaleax commented Mar 12, 2019

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

joyeecheung commented Mar 12, 2019

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

addaleax commented Mar 12, 2019

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

joyeecheung commented Mar 12, 2019

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

joyeecheung commented Mar 18, 2019

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

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 18, 2019

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

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 18, 2019

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 <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@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 <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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 <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@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

@addaleax addaleax addaleax approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+2 more reviewers

@jdalton jdalton jdalton left review comments

@eugeneo eugeneo eugeneo approved these changes

Reviewers whose approvals may not affect merge requirements

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