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

[v11.x] backport warning handler refactoring #26670

Closed

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 14, 2019

process: handle node --debug deprecation in pre-execution

In addition, shim process._deprecatedDebugBrk in pre-execution.
This is a non-semver-major v11.x backport for
#25828.

Refs: #25828

process: call prepareMainThreadExecution in node inspect

Since we should treat the node-inspect as third-party
user code.

PR-URL: #26466
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

process: set up process warning handler in pre-execution

Since it depends on environment variables.

PR-URL: #26466
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

process: handle process.env.NODE_V8_COVERAGE in pre-execution

Since this depends on environment variable, and the worker threads
do not need to persist the variable value because they cannot
switch cwd.

PR-URL: #26466
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@joyeecheung
Copy link
Member Author

cc @BridgeAR

@joyeecheung
Copy link
Member Author

Ah, didn't see #26662

@refack did you do something similar to the first commit added in this PR? otherwise the tests would fail

@refack
Copy link
Contributor

refack commented Mar 14, 2019

I did something similar to eef44c4
in
93795d9

(since it's a backport I didn't go for finesse ;)

@BridgeAR
Copy link
Member

@BridgeAR
Copy link
Member

@joyeecheung this seems to fail some tests.

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 15, 2019

hmm, I just realized we can't just semi-backport #25828 like this, because currently there is no way for the JS land to precisely know about the invalid deprecation that don't exit the process directly (--inspect --debug-brk), which also blocks #26517 if we want to shim process._invalidDebug and process._deprecatedDebugBrk precisely on v11.x. So the question is:

  1. Is it worth it to add some separate logic in v11.x that informs the JS land about the precise values of --debug-brk? That would result in conflicts for any future changes to internal/options.js and src/node_options.*.
  2. Or is it okay to just throw warning and set process._deprecatedDebugBrk to true instead of exiting for --inspect-brk --debug?

I am leaning towards 2, because it doesn't seem to hurt much to not exit the process with some combinations of options that probably should be unused by now.

@joyeecheung
Copy link
Member Author

Huh, wait, looks like even before #25828 there was already no way from C++ to tell the difference between --inspect-brk --debug and --debug-brk --inspect. Both of them would result in a warning, and nobody really had noticed the difference in v11 so far..so I guess we can just go with option 2.

@refack The additional patch should also shim process._deprecatedDebugBrk in the user land in order to unblock #26517. (we don't need to shim process._invalidDebug though because it will exit the process directly)

In addition, shim `process._deprecatedDebugBrk` in pre-execution.
This is a non-semver-major v11.x backport for
nodejs#25828.

Refs: nodejs#25828
Since we should treat the node-inspect as third-party
user code.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Since it depends on environment variables.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Since this depends on environment variable, and the worker threads
do not need to persist the variable value because they cannot
switch cwd.

PR-URL: nodejs#26466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
In addition, shim `process._deprecatedDebugBrk` in pre-execution.
This is a non-semver-major v11.x backport for
nodejs#25828.

PR-URL: nodejs#26670
Refs: nodejs#25828
targos pushed a commit that referenced this pull request Mar 27, 2019
In addition, shim `process._deprecatedDebugBrk` in pre-execution.
This is a non-semver-major v11.x backport for
#25828.

PR-URL: #26670
Refs: #25828
@targos
Copy link
Member

targos commented Mar 27, 2019

Landed the first commit in v11.x-staging.
Others landed with #26662.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants