module: improve detection try catch - #52242
Conversation
Sorry, something went wrong.
…corate the error when not retrying
|
Review requested:
|
Sorry, something went wrong.
|
@nodejs/v8 When I run ./node test-debugger-exceptions.js on this branch, it errors on the last line of this section: // Next run: With `breakOnUncaught` it only pauses on the 2nd exception.
await cli.command('breakOnUncaught');
await cli.stepCommand('r'); // Also, the setting survives the restart.
await cli.waitForInitialBreak();
assert.deepStrictEqual(cli.breakInfo, { filename: script, line: 1 });
await cli.stepCommand('c');
assert.ok(cli.output.includes(`exception in ${script}:9`));If I log that cli.output, I see this: exception in node:internal/modules/run_main:171
169 const { Module } = cjsLoader;
170
>171 tryCatchRethrow(() => {
172 // Module._load is the monkey-patchable CJS module loader.
173 Module._load(main, null, true);
Which should be the fixture code, not Node internal code. I guess that somehow node inspect is adding its own try_catch_scope or similar that’s a level higher than the one created by tryCatchRethrow, and even when tryCatchRethrow catches an exception, that exception is also caught by the inspector? Can anyone point me to where this code lives, and any ideas on how to avoid it? I don’t want the inspector ever catching on tryCatchRethrow, only on user code exceptions that are rethrown by it. |
Sorry, something went wrong.
| const source = cjsLoader.entryPointSource; | ||
|
|
||
| tryCatchRethrow(() => { | ||
| // Module._load is the monkey-patchable CJS module loader. |
There was a problem hiding this comment.
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).
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
Sorry for not responding any earlier. Two questions I would have is:
But maybe that's moot if the alternate implementation approach that @joyeecheung mentioned shakes out. |
Sorry, something went wrong.
Yes, fortunately I think it's moot now thanks to Joyee's PR. |
Sorry, something went wrong.
This refactors the try/catch added in #52093 to happen in C++, so that the rethrown error preserves the same origin, source arrow and source map-translated stack trace. This gets us much closer to being able to unflag --experimental-detect-module and have all the tests pass. The C++ was written by @targos
There’s currently only one test failing on this branch, and I’d love some help getting it to pass. ./node test/parallel/test-debugger-exceptions.js is failing because in node inspect, for some reason the inspector sees the exception as being thrown by the new TryCatchRethrow rather than the rethrown exception. If anyone can figure out why this is and what we can do about it, I would be very grateful. cc @jkrems @nodejs/loaders @nodejs/inspector @nodejs/v8-inspector