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

Unable to catch error thrown during require in node.js v22 (works in v20) #56115

Closed
davidfirst opened this issue Dec 2, 2024 · 2 comments · Fixed by #56122
Closed

Unable to catch error thrown during require in node.js v22 (works in v20) #56115

davidfirst opened this issue Dec 2, 2024 · 2 comments · Fixed by #56122
Labels
confirmed-bug Issues with confirmed bugs. module Issues and PRs related to the module subsystem.

Comments

@davidfirst
Copy link

davidfirst commented Dec 2, 2024

Version

v22.11.0

Platform

Darwin Davids-MacBook-Pro.local 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:14:30 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

Create a new directory and add two files: bar.js and foo.js.

bar.js

try {
  require('./foo');
} catch (err) {
  console.log('got an error', err);
}

foo.js

Object.defineProperty(exports, "__esModule", { value: true });
const module_1 = require("module");
const require = (0, module_1.createRequire)(import.meta.url);

Run node bar.js.

Observe that with node v22 it exits with code 1 while with node v20 it exits with code 0.

How often does it reproduce? Is there a required condition?

It reproduces constantly.

What is the expected behavior? Why is that the expected behavior?

I expect node to exit with code 0 because I wrapped the require with try/catch.

What do you see instead?

node exits with code 1.

Additional information

In practice, bar.js is my CLI tool, and foo.js is a user-created plugin over which I have no control.

@joyeecheung
Copy link
Member

It seems in this case the module being loaded by require() is recognized as ESM (due to the presence of const require) by syntax detection and went into require(esm), which is kind of odd because require(esm) is still behind a flag in 22.11.0, it might be another glitch from the syntax detection unflagging.

But anyway, it makes the process exit because the evaluation promise is created by V8 and once rejection happens it goes straight into the unhandled rejection handler, so we need to do some fixup later to remove it from the pending unhandled rejections.

@joyeecheung
Copy link
Member

Fix in #56122 (there is a separate bug in which require(esm) is not disabled if the module syntax detection indicates the module is ESM, but considering this has been happening on v22.x for quite a while, it is probably not too urgent to fix it).

@joyeecheung joyeecheung added confirmed-bug Issues with confirmed bugs. module Issues and PRs related to the module subsystem. labels Dec 3, 2024
targos pushed a commit that referenced this issue Dec 6, 2024
Previously the implemention of require(esm) only converted the
rejected promise from module evaluation into an error, but the
rejected promise was still treated as a pending unhandled
rejection by the promise rejection callback, because the promise
is created by V8 internals and we don't get a chance to mark
it as handled, so the rejection incorrectly marked as unhandled
would still go through unhandled rejection handling (if no
global listener is set, the default handling would print a warning
and make the Node.js instance exit with 1).

This patch fixes it by calling into the JS promise rejection
callback to mark the evalaution rejection handled so that
it doesn't go through unhandled rejection handling.

PR-URL: #56122
Fixes: #56115
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this issue Dec 13, 2024
Previously the implemention of require(esm) only converted the
rejected promise from module evaluation into an error, but the
rejected promise was still treated as a pending unhandled
rejection by the promise rejection callback, because the promise
is created by V8 internals and we don't get a chance to mark
it as handled, so the rejection incorrectly marked as unhandled
would still go through unhandled rejection handling (if no
global listener is set, the default handling would print a warning
and make the Node.js instance exit with 1).

This patch fixes it by calling into the JS promise rejection
callback to mark the evalaution rejection handled so that
it doesn't go through unhandled rejection handling.

PR-URL: #56122
Fixes: #56115
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this issue Dec 13, 2024
Previously the implemention of require(esm) only converted the
rejected promise from module evaluation into an error, but the
rejected promise was still treated as a pending unhandled
rejection by the promise rejection callback, because the promise
is created by V8 internals and we don't get a chance to mark
it as handled, so the rejection incorrectly marked as unhandled
would still go through unhandled rejection handling (if no
global listener is set, the default handling would print a warning
and make the Node.js instance exit with 1).

This patch fixes it by calling into the JS promise rejection
callback to mark the evalaution rejection handled so that
it doesn't go through unhandled rejection handling.

PR-URL: #56122
Fixes: #56115
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this issue Dec 13, 2024
Previously the implemention of require(esm) only converted the
rejected promise from module evaluation into an error, but the
rejected promise was still treated as a pending unhandled
rejection by the promise rejection callback, because the promise
is created by V8 internals and we don't get a chance to mark
it as handled, so the rejection incorrectly marked as unhandled
would still go through unhandled rejection handling (if no
global listener is set, the default handling would print a warning
and make the Node.js instance exit with 1).

This patch fixes it by calling into the JS promise rejection
callback to mark the evalaution rejection handled so that
it doesn't go through unhandled rejection handling.

PR-URL: #56122
Fixes: #56115
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this issue Dec 18, 2024
Previously the implemention of require(esm) only converted the
rejected promise from module evaluation into an error, but the
rejected promise was still treated as a pending unhandled
rejection by the promise rejection callback, because the promise
is created by V8 internals and we don't get a chance to mark
it as handled, so the rejection incorrectly marked as unhandled
would still go through unhandled rejection handling (if no
global listener is set, the default handling would print a warning
and make the Node.js instance exit with 1).

This patch fixes it by calling into the JS promise rejection
callback to mark the evalaution rejection handled so that
it doesn't go through unhandled rejection handling.

PR-URL: #56122
Fixes: #56115
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants