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

lib: explicitly initialize debuglog during bootstrap #26468

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

lib: move format and formatWithOptions into internal/util/inspect.js

So these can be required without requiring the whole util.js.

process: call prepareMainThreadExecution in all main thread scripts

lib: explicitly initialize debuglog during bootstrap

This patch splits the implementation of util.debuglog into a
separate file and explicitly initialize it during pre-execution
since the initialization depends on environment variables.
Also delays the call to debuglog in modules that are loaded during
bootstrap to make sure we only access the environment variable
during pre-execution.

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

So these can be required without requiring the whole `util.js`.
This patch splits the implementation of util.debuglog into a
separate file and explicitly initialize it during pre-execution
since the initialization depends on environment variables.
Also delays the call to `debuglog` in modules that are loaded during
bootstrap to make sure we only access the environment variable
during pre-execution.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 6, 2019
@joyeecheung
Copy link
Member Author

lib/internal/util/debuglog.js Show resolved Hide resolved
lib/internal/util/debuglog.js Show resolved Hide resolved
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 7, 2019
@joyeecheung
Copy link
Member Author

Landed in f617a73...b05fd4b

@joyeecheung joyeecheung closed this Mar 8, 2019
pull bot pushed a commit to Rachelmorrell/node that referenced this pull request Mar 8, 2019
So these can be required without requiring the whole `util.js`.

PR-URL: nodejs#26468
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
pull bot pushed a commit to Rachelmorrell/node that referenced this pull request Mar 8, 2019
PR-URL: nodejs#26468
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
pull bot pushed a commit to Rachelmorrell/node that referenced this pull request Mar 8, 2019
This patch splits the implementation of util.debuglog into a
separate file and explicitly initialize it during pre-execution
since the initialization depends on environment variables.
Also delays the call to `debuglog` in modules that are loaded during
bootstrap to make sure we only access the environment variable
during pre-execution.

PR-URL: nodejs#26468
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

It looks like this may have broken our downstream modules fork

nodejs/ecmascript-modules#53

More to come as I dig in and figure out exactly what is going on

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 9, 2019

@MylesBorins My guess is there's something in the fork that loads debug during bootstrapping which is exactly what this PR tries to forbid. You can instantiate debuglog lazily only when debug is called, similar to how its done in this PR in multiple modules loaded during bootstrap. e.g. for streams:

// Don't do this in the top scope if it's loaded during bootstrap
const debug = require('util').debuglog('stream');

// Do this
let debuglog;
function debug(...args) {
  if (!debuglog) {
    debuglog = require('internal/util/debuglog').debuglog('stream');
  }
  debuglog(...args);
}

@MylesBorins
Copy link
Contributor

thanks @joyeecheung. it looks like the file being required uses the above pattern... and I had a bit of an issue tracking down where the call to debug was happening in the wrong order. I opted instead to lazyload the instantiation of the CJSLoader

see nodejs/ecmascript-modules@02bcd09 for attempted fix. Have a feeling this requires more attention, but appears good enough to move things forward for now

@BridgeAR
Copy link
Member

This does land land cleanly on v11.x. It relies on #26466. As soon as that is backported, this should also land cleanly.

@refack
Copy link
Contributor

refack commented Mar 14, 2019

Cross-ref: PR to backport #26466 #26662

targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
So these can be required without requiring the whole `util.js`.

PR-URL: nodejs#26468
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
PR-URL: nodejs#26468
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
This patch splits the implementation of util.debuglog into a
separate file and explicitly initialize it during pre-execution
since the initialization depends on environment variables.
Also delays the call to `debuglog` in modules that are loaded during
bootstrap to make sure we only access the environment variable
during pre-execution.

PR-URL: nodejs#26468
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants