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.

Sign up for GitHub

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

Jump to bottom

module: improve detection try catch #52242

Closed
wants to merge 9 commits into from
Closed

module: improve detection try catch #52242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c7813df
module: improve detection try catch
GeoffreyBooth Mar 21, 2024
23adfbb
restore original enrichment location
GeoffreyBooth Mar 23, 2024
179152a
Move try/catch outside of flag
GeoffreyBooth Mar 23, 2024
0f28c7e
Update message tests
GeoffreyBooth Mar 23, 2024
b66b39e
Move error enrichment back; fix condition
GeoffreyBooth Mar 23, 2024
78ca8e3
update test fixture/snapshot stack traces
GeoffreyBooth Mar 28, 2024
a747816
update pseudo-tty snapshots
GeoffreyBooth Mar 28, 2024
060bf7e
Improve preserving entry point source so that we properly warn and de…
GeoffreyBooth Mar 28, 2024
d59bc95
lint
GeoffreyBooth Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions lib/internal/modules/cjs/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1338,15 +1338,8 @@ function wrapSafe(filename, content, cjsModuleInstance, codeCache) {
return result;
} catch (err) {
if (process.mainModule === cjsModuleInstance) {
if (getOptionValue('--experimental-detect-module')) {
// For the main entry point, cache the source to potentially retry as ESM.
module.exports.entryPointSource = content;
} else {
// We only enrich the error (print a warning) if we're sure we're going to for-sure throw it; so if we're
// retrying as ESM, wait until we know whether we're going to retry before calling `enrichCJSError`.
const { enrichCJSError } = require('internal/modules/esm/translators');
enrichCJSError(err, content, filename);
}
// For the main entry point, cache the source to potentially retry as ESM; or to decorate the error.
module.exports.entryPointSource = content;
}
throw err;
}
Expand Down
28 changes: 17 additions & 11 deletions lib/internal/modules/run_main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const {
hasUncaughtExceptionCaptureCallback,
} = require('internal/process/execution');
const {
tryCatchRethrow,
triggerUncaughtException,
} = internalBinding('errors');
const {
Expand Down Expand Up @@ -166,26 +167,31 @@ function executeUserEntryPoint(main = process.argv[1]) {
if (!useESMLoader) {
const cjsLoader = require('internal/modules/cjs/loader');
const { Module } = cjsLoader;
if (getOptionValue('--experimental-detect-module')) {
try {
// Module._load is the monkey-patchable CJS module loader.
Module._load(main, null, true);
} catch (error) {
const source = cjsLoader.entryPointSource;

tryCatchRethrow(() => {
// Module._load is the monkey-patchable CJS module loader.
Copy link
Member

@joyeecheung joyeecheung Apr 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Calling back from C++ to JS would have a non-trivial performance impact. I think the proper way to deal with this without all the weird error handling should be handling ESM directly in the CJS loader when the compilation fails and the source looks like ESM (and if it's async, simply ignoring the exports of the entry point/making it some kind of dummy object should be fine, because no one really needs that, or if they do weird things like process.mainModule.exports, we can warn/error when the entry point is async ESM).

Copy link
Member

@joyeecheung joyeecheung Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have a branch here built on top of #52413 that implements the idea above + require(esm) detection. This moves the reparsing into C++ and also handles ESM entrypoints in-place in the CJS loader (by just doing cascadedLoader.import() and making the CJS exports a proxy that emits a warning, technically I think we can just delete process.mainModule in that case, then it would probably be impossible to reach to the exports of the entry point without some hacky monkey patch that maybe no one really does), so it passes ./node test/parallel/test-debugger-exceptions.js when the flag is flipped because the error doesn't bubble all the way up.

Module._load(main, null, true);
}, (error) => {
const source = cjsLoader.entryPointSource;
if (getOptionValue('--experimental-detect-module')) {
if (!source) {
return true; // Rethrow original exception.
}
const { shouldRetryAsESM } = require('internal/modules/helpers');
retryAsESM = shouldRetryAsESM(error.message, source);
// In case the entry point is a large file, such as a bundle,
// ensure no further references can prevent it being garbage-collected.
cjsLoader.entryPointSource = undefined;
if (!retryAsESM) {
}

if (!retryAsESM) {
if (source) {
const { enrichCJSError } = require('internal/modules/esm/translators');
enrichCJSError(error, source, resolvedMain);
throw error;
}
return true; // Rethrow original exception.
}
} else { // `--experimental-detect-module` is not passed
Module._load(main, null, true);
}
});
}

if (useESMLoader || retryAsESM) {
Expand Down
35 changes: 35 additions & 0 deletions src/node_errors.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,39 @@ using v8::String;
using v8::Undefined;
using v8::Value;

// Takes two JS callbacks, and calls the first one in a TryCatch scope.
// If the first one throws, call the second one with the exception.
// If the second one returns true, rethrow the original exception.
// This is to keep the location of the original throw in JS.
static void TryCatchRethrow(const FunctionCallbackInfo<Value>& args) {
CHECK_EQ(args.Length(), 2);
CHECK(args[0]->IsFunction());
CHECK(args[1]->IsFunction());

Local<Function> try_fn = args[0].As<Function>();
Local<Function> catch_fn = args[1].As<Function>();

Isolate* isolate = args.GetIsolate();
Environment* env = Environment::GetCurrent(isolate);
Local<Context> context = isolate->GetCurrentContext();

TryCatchScope try_catch_scope(env);
USE(try_fn->Call(context, Undefined(isolate), 0, nullptr));

if (try_catch_scope.HasCaught()) {
Local<Value> exception = try_catch_scope.Exception();
Local<Value> argv[] = {exception};
Local<Value> result;
if (!catch_fn->Call(context, Undefined(isolate), 1, argv)
.ToLocal(&result)) {
return;
}
if (result->IsTrue()) {
try_catch_scope.ReThrow();
}
}
}

bool IsExceptionDecorated(Environment* env, Local<Value> er) {
if (!er.IsEmpty() && er->IsObject()) {
Local<Object> err_obj = er.As<Object>();
Expand Down Expand Up @@ -1089,6 +1122,7 @@ static void TriggerUncaughtException(const FunctionCallbackInfo<Value>& args) {
}

void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(TryCatchRethrow);
registry->Register(SetPrepareStackTraceCallback);
registry->Register(SetGetSourceMapErrorSource);
registry->Register(SetSourceMapsEnabled);
Expand All @@ -1102,6 +1136,7 @@ void Initialize(Local<Object> target,
Local<Value> unused,
Local<Context> context,
void* priv) {
SetMethod(context, target, "tryCatchRethrow", TryCatchRethrow);
SetMethod(context,
target,
"setPrepareStackTraceCallback",
Expand Down
2 changes: 1 addition & 1 deletion test/es-module/test-esm-cjs-load-error-note.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ describe('ESM: Errors for unexpected exports', { concurrency: true }, () => {
},
]
) {
it(`should ${includeNote ? '' : 'NOT'} include note`, async () => {
it(`should${includeNote ? '' : ' NOT'} include note`, async () => {
const { code, stderr } = await spawnPromisified(execPath, [filePath]);

assert.strictEqual(code, 1);
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/console/console.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ Trace: foo
at *
at *
at *
at *
1 change: 1 addition & 0 deletions test/fixtures/errors/force_colors.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ Error: Should include grayed stack trace
 at *
 at *
 at *
 at *

Node.js *
1 change: 1 addition & 0 deletions test/fixtures/errors/promise_always_throw_unhandled.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ Error: One
at *
at *
at *
at *

Node.js *
1 change: 1 addition & 0 deletions test/fixtures/errors/promise_unhandled_warn_with_error.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@
at *
at *
at *
at *
(Use `node --trace-warnings ...` to show where the warning was created)
(node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https:*nodejs.org*api*cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
2 changes: 2 additions & 0 deletions test/fixtures/errors/unhandled_promise_trace_warnings.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
at *
at *
at *
at *
(node:*) Error: This was rejected
at *
at *
Expand All @@ -17,6 +18,7 @@
at *
at *
at *
at *
(node:*) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)
at *
at *
Expand Down
2 changes: 2 additions & 0 deletions test/fixtures/test-runner/output/abort.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ not ok 2 - promise abort signal
*
*
*
*
...
# Subtest: callback timeout signal
# Subtest: ok 1
Expand Down Expand Up @@ -272,6 +273,7 @@ not ok 4 - callback abort signal
*
*
*
*
...
1..4
# tests 22
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/test-runner/output/abort_suite.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ not ok 2 - describe abort signal
*
*
*
*
...
1..2
# tests 9
Expand Down
1 change: 1 addition & 0 deletions test/message/assert_throws_stack.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
at *
at *
at *
at *
at * {
generatedMessage: true,
code: 'ERR_ASSERTION',
Expand Down
1 change: 1 addition & 0 deletions test/message/internal_assert.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Please open an issue with this stack trace at https://github.com/nodejs/node/iss
at *
at *
at *
at *
at * {
code: 'ERR_INTERNAL_ASSERTION'
}
Expand Down
1 change: 1 addition & 0 deletions test/message/internal_assert_fail.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Please open an issue with this stack trace at https://github.com/nodejs/node/iss
at *
at *
at *
at *
at * {
code: 'ERR_INTERNAL_ASSERTION'
}
Expand Down
30 changes: 20 additions & 10 deletions test/message/util-inspect-error-cause.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Error: Number error cause
at *
at *
at *
at *
at * {
[cause]: 42
}
Expand All @@ -15,6 +16,7 @@ Error: Object cause
at *
at *
at *
at *
at * {
[cause]: {
message: 'Unique',
Expand All @@ -30,6 +32,7 @@ Error: undefined cause
at *
at *
at *
at *
at * {
[cause]: undefined
}
Expand All @@ -40,6 +43,7 @@ Error: cause that throws
at *
at *
at *
at *
at * {
[cause]: [Getter]
}
Expand All @@ -49,7 +53,7 @@ RangeError: New Stack Frames
[cause]: FoobarError: Individual message
at *
*[90m at *[39m
*[90m ... 4 lines matching cause stack trace ...*[39m
*[90m ... 5 lines matching cause stack trace ...*[39m
*[90m at *[39m {
status: *[32m'Feeling good'*[39m,
extraProperties: *[32m'Yes!'*[39m,
Expand All @@ -61,17 +65,18 @@ RangeError: New Stack Frames
*[90m at *[39m
*[90m at *[39m
*[90m at *[39m
*[90m at *[39m
}
}
Error: Stack causes
at *
*[90m at *[39m
*[90m ... 4 lines matching cause stack trace ...*[39m
*[90m ... 5 lines matching cause stack trace ...*[39m
*[90m at *[39m {
[cause]: FoobarError: Individual message
at *
*[90m at *[39m
*[90m ... 4 lines matching cause stack trace ...*[39m
*[90m ... 5 lines matching cause stack trace ...*[39m
*[90m at *[39m {
status: *[32m'Feeling good'*[39m,
extraProperties: *[32m'Yes!'*[39m,
Expand All @@ -83,6 +88,7 @@ Error: Stack causes
*[90m at *[39m
*[90m at *[39m
*[90m at *[39m
*[90m at *[39m
}
}
RangeError: New Stack Frames
Expand All @@ -91,12 +97,12 @@ RangeError: New Stack Frames
[cause]: Error: Stack causes
at *
*[90m at *[39m
*[90m ... 4 lines matching cause stack trace ...*[39m
*[90m ... 5 lines matching cause stack trace ...*[39m
*[90m at *[39m {
[cause]: FoobarError: Individual message
at *
*[90m at *[39m
*[90m ... 4 lines matching cause stack trace ...*[39m
*[90m ... 5 lines matching cause stack trace ...*[39m
*[90m at *[39m {
status: *[32m'Feeling good'*[39m,
extraProperties: *[32m'Yes!'*[39m,
Expand All @@ -108,6 +114,7 @@ RangeError: New Stack Frames
*[90m at *[39m
*[90m at *[39m
*[90m at *[39m
*[90m at *[39m
}
}
}
Expand All @@ -117,7 +124,7 @@ RangeError: New Stack Frames
[cause]: FoobarError: Individual message
at *
at *
... 4 lines matching cause stack trace ...
... 5 lines matching cause stack trace ...
at * {
status: 'Feeling good',
extraProperties: 'Yes!',
Expand All @@ -129,17 +136,18 @@ RangeError: New Stack Frames
at *
at *
at *
at *
}
}
Error: Stack causes
at *
at *
... 4 lines matching cause stack trace ...
... 5 lines matching cause stack trace ...
at * {
[cause]: FoobarError: Individual message
at *
at *
... 4 lines matching cause stack trace ...
... 5 lines matching cause stack trace ...
at *
status: 'Feeling good',
extraProperties: 'Yes!',
Expand All @@ -151,6 +159,7 @@ Error: Stack causes
at *
at *
at *
at *
}
}
RangeError: New Stack Frames
Expand All @@ -159,12 +168,12 @@ RangeError: New Stack Frames
[cause]: Error: Stack causes
at *
at *
... 4 lines matching cause stack trace ...
... 5 lines matching cause stack trace ...
at * {
[cause]: FoobarError: Individual message
at *
at *
... 4 lines matching cause stack trace ...
... 5 lines matching cause stack trace ...
at * {
status: 'Feeling good',
extraProperties: 'Yes!',
Expand All @@ -176,6 +185,7 @@ RangeError: New Stack Frames
at *
at *
at *
at *
}
}
}
Loading
Loading