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

module: ignore resolution failures for inspect-brk #30336

Closed
wants to merge 3 commits into from

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Nov 8, 2019

The resolution for the main entry point may fail when the resolution requires
a preloaded module to be executed first (for example when adding new extensions
to the resolution process). Silently skipping such failures allow us to defer
the resolution as long as needed without having any adverse change (since the
main entry point won't resolve anyway if it really can't be resolved at all).

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

@arcanis
Copy link
Contributor Author

arcanis commented Nov 11, 2019

Ping @guybedford? (I've seen that you made most recent changes in this file)

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

This entire code path is a little messy, ideally it could be more like the process.mainModule === cjsModuleInstance comparison to avoid the need to double resolve at all.

If a refactoring could be made to avoid that resolution entirely that would be much much nicer (although would likely require another function argument), but otherwise this approach makes sense to me.

@nodejs-github-bot
Copy link
Collaborator

// eslint-disable-next-line no-unused-vars
const common = require('../common');

require.extensions['.ext'] = require.extensions['.js'];
Copy link
Member

Choose a reason for hiding this comment

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

This file should be in test/fixtures instead. Ditto for the .ext file.

@@ -1091,14 +1091,17 @@ Module.prototype._compile = function(content, filename) {
if (!resolvedArgv) {
// We enter the repl if we're not given a filename argument.
if (process.argv[1]) {
resolvedArgv = Module._resolveFilename(process.argv[1], null, false);
// The resolution may fail if it requires a preload script
Copy link
Member

Choose a reason for hiding this comment

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

I am confused - this only fails if the preload script tries to extend the module resolver right? This comment seems to imply that --require alone can cause failures.

Also it would be better if we could assert the condition under which we allow this to fail. Array.isArray(getOptionValue('--require')) is part of that but there should be more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes for the first point.

I'm not sure I understand what you mean in the second, can you detail?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean in the second, can you detail?

I meant we should at least assert(Array.isArray(getOptionValue('--require'))) in the catch block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not convinced - this codepath (where resolveArgv is empty and _resolveFilename fails but _compile is called) is only possible for the preloaded scripts anyway (since otherwise _compile won't be called at all if _resolvedFilename fails on the real main entry script).

That said, if you still think it's a blocker, I can add it.

Copy link
Member

@joyeecheung joyeecheung Nov 19, 2019

Choose a reason for hiding this comment

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

..is only possible for the preloaded scripts anyway

Yes, exactly, and that's why it should be an assertion - to prevent people from changing this invariant unexpectedly in the future. That happens a lot when code are being moved/copy-pasted around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 👍

Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is redundant now?

@joyeecheung
Copy link
Member

If a refactoring could be made to avoid that resolution entirely that would be much much nicer (although would likely require another function argument)

At this point, it seems we could specialize the handling of --require a bit since at that point the users cannot monkey-patch module yet and there are certain exceptions (like this one) that should be considered.

@arcanis
Copy link
Contributor Author

arcanis commented Nov 14, 2019

If a refactoring could be made to avoid that resolution entirely that would be much much nicer (although would likely require another function argument), but otherwise this approach makes sense to me.

I tried to avoid changing the API as much as possible to avoid potential disruption. It's a fairly small change that can't break anything that was working before, which I think is safer in this situation than a full refactoring. What do you think?

This entire code path is a little messy, ideally it could be more like the process.mainModule === cjsModuleInstance comparison to avoid the need to double resolve at all.

I think we can't do this - it would fail for this kind of setup (but who does that, arguably):

node --inspect-brk --require ./foo.js ./foo.js

In this situation you will want to break inside foo.js during the preload step, since it won't get executed as main entry point (because it's in the cache). This makes it impossible to wait for the preload list to be fully executed, or to wait for mainModule or similar to be present.

Frankly it seems a super fringe use case, but I guess that's the rational behind the way the code is written otherwise I'm not sure why the ahead-of-time resolution was needed in the first place 🤔

The resolution for the main entry point may fail when the resolution
requires a preloaded module to be executed first (for example when
adding new extensions to the resolution process). Silently skipping
such failures allow us to defer the resolution as long as needed
without having any adverse change (since the main entry point won't
resolve anyway if it really can't be resolved at all).
@guybedford
Copy link
Contributor

I think we can't do this - it would fail for this kind of setup (but who does that, arguably):

Wait, doesn't the current logic also fail for this case?

@guybedford
Copy link
Contributor

In the esm resolver we have an isMain flag which is true through the load operation for the module that corresponds to the main entry into Node.js only - so that's the sort of logic I mean as an alternative.

@arcanis
Copy link
Contributor Author

arcanis commented Nov 16, 2019

Wait, doesn't the current logic also fail for this case?

No, because the break is execute inside _compile, so even for preloaded module.

In the esm resolver we have an isMain flag which is true through the load operation for the module that corresponds to the main entry into Node.js only - so that's the sort of logic I mean as an alternative.

There's also a isMain in the cjs loader (in _load). Still, the case described above wouldn't work anymore since the isMain field is only set when executing the main entry point, so not preloaded scripts. The resolution that happens in _compile is from my understanding a way to detect whether the script being compiled will be the main script - before it actually is.

@guybedford
Copy link
Contributor

Ah so you suspect this code path was exactly to handle this edge case in the first place, where the main itself could be in the module cache. Although it does seem very much an edge case as opposed to one to be justifying the base-level semantics on, but I guess that is where things are indeed.

@arcanis
Copy link
Contributor Author

arcanis commented Nov 25, 2019

Ping?

@nodejs-github-bot
Copy link
Collaborator

@arcanis
Copy link
Contributor Author

arcanis commented Dec 2, 2019

I'm not sure if the failing tests are related to my diff (I'd assume they aren't, since they're passing most platforms) - can I get some help?

@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor

It doesn't look related to me, I've re-triggered the failed builds.

@guybedford
Copy link
Contributor

The test failure here seems to be in async hooks -

test.node-api/test_make_callback/test-async-hooks-gcable
crashed (-11)

will try again and see if it works this time.

@guybedford
Copy link
Contributor

Actually no, the Travis report is wrong - this is all green.

guybedford pushed a commit that referenced this pull request Dec 5, 2019
The resolution for the main entry point may fail when the resolution
requires a preloaded module to be executed first (for example when
adding new extensions to the resolution process). Silently skipping
such failures allow us to defer the resolution as long as needed
without having any adverse change (since the main entry point won't
resolve anyway if it really can't be resolved at all).

PR-URL: #30336
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@guybedford
Copy link
Contributor

Landed in 1549c8e.

@guybedford guybedford closed this Dec 5, 2019
@guybedford guybedford mentioned this pull request Dec 5, 2019
2 tasks
targos pushed a commit that referenced this pull request Dec 9, 2019
The resolution for the main entry point may fail when the resolution
requires a preloaded module to be executed first (for example when
adding new extensions to the resolution process). Silently skipping
such failures allow us to defer the resolution as long as needed
without having any adverse change (since the main entry point won't
resolve anyway if it really can't be resolved at all).

PR-URL: #30336
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
The resolution for the main entry point may fail when the resolution
requires a preloaded module to be executed first (for example when
adding new extensions to the resolution process). Silently skipping
such failures allow us to defer the resolution as long as needed
without having any adverse change (since the main entry point won't
resolve anyway if it really can't be resolved at all).

PR-URL: #30336
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
The resolution for the main entry point may fail when the resolution
requires a preloaded module to be executed first (for example when
adding new extensions to the resolution process). Silently skipping
such failures allow us to defer the resolution as long as needed
without having any adverse change (since the main entry point won't
resolve anyway if it really can't be resolved at all).

PR-URL: #30336
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 15, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 18, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
* chore: bump node in DEPS to v12.16.0

* Fixup asar support setup patch

nodejs/node#30862

* Fixup InternalCallbackScope patch

nodejs/node#30236

* Fixup GN buildfiles patch

nodejs/node#30755

* Fixup low-level hooks patch

nodejs/node#30466

* Fixup globals require patch

nodejs/node#31643

* Fixup process stream patch

nodejs/node#30862

* Fixup js2c modification patch

nodejs/node#30755

* Fixup internal fs override patch

nodejs/node#30610

* Fixup context-aware warn patch

nodejs/node#30336

* Fixup Node.js with ltcg config

nodejs/node#29388

* Fixup oaepLabel patch

nodejs/node#30917

* Remove redundant ESM test patch

nodejs/node#30997

* Remove redundant cli flag patch

nodejs/node#30466

* Update filenames.json

* Remove macro generation in GN build files

nodejs/node#30755

* Fix some compilation errors upstream

* Add uvwasi to deps

nodejs/node#30258

* Fix BoringSSL incompatibilities

* Fixup linked module patch

nodejs/node#30274

* Add missing sources to GN uv build

libuv/libuv#2347

* Patch some uvwasi incompatibilities

* chore: bump Node.js to v12.6.1

* Remove mark_arraybuffer_as_untransferable.patch

nodejs/node#30549

* Fix uvwasi build failure on win

* Fixup --perf-prof cli option error

* Fixup early cjs module loading

* fix: initialize diagnostics properly

nodejs/node#30025

* Disable new esm syntax specs

nodejs/node#30219

* Fixup v8 weakref hook spec

nodejs/node#29874

* Fix async context timer issue

* Disable monkey-patch-main spec

It relies on nodejs/node#29777, and we don't
override prepareStackTrace.

* Disable new tls specs

nodejs/node#23188

We don't support much of TLS owing to schisms between BoringSSL and
OpenSSL.

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
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.

4 participants