debugger: refactor internal/inspector/_inspect to use more primordials
#38406
Conversation
github-actions
bot
added
inspector
| child.stderr.on('data', (chunk) => childPrint(chunk, 'stderr')); | ||
|
|
||
| let output = ''; | ||
| return new Promise((resolve) => { |
There was a problem hiding this comment.
Should this go ahead and use return await Promise so that any possible error stack would include the runScript frame?
There was a problem hiding this comment.
I'm not sure I follow, isn't await <promise> and return <promise> similar when it's the last expression on an async function?
There was a problem hiding this comment.
Yes and no. With return <promise> if <promise> rejects, the error stack trace will not include the wrapping async function (in this case runScript). However, when using return await <promise> the error stack will contain runScript.
There was a problem hiding this comment.
Am I doing this wrong? It seems the stack trace does contain the wrapping function in the dummy test cases I've came up with..
Also with node:
$ node -e '(async function runScripts(){return new Promise((_,r)=>{r(new Error)})})()'
[eval]:1
(async function runScripts(){return new Promise((_,r)=>{r(new Error)})})()
^
Error
at [eval]:1:59
at new Promise (<anonymous>)
at runScripts ([eval]:1:37)
at [eval]:1:73
at Script.runInThisContext (node:vm:131:12)
at Object.runInThisContext (node:vm:308:38)
at node:internal/process/execution:81:19
at [eval]-wrapper:6:22
at evalScript (node:internal/process/execution:80:60)
at node:internal/main/eval_string:27:3
$ node -e '(async function runScripts(){return new Promise(()=>{throw new Error})})()'
[eval]:1
(async function runScripts(){return new Promise(()=>{throw new Error})})()
^
Error
at [eval]:1:60
at new Promise (<anonymous>)
at runScripts ([eval]:1:37)
at [eval]:1:73
at Script.runInThisContext (node:vm:131:12)
at Object.runInThisContext (node:vm:308:38)
at node:internal/process/execution:81:19
at [eval]-wrapper:6:22
at evalScript (node:internal/process/execution:80:60)
at node:internal/main/eval_string:27:3There was a problem hiding this comment.
Yeah looking at it again, in this particular case returning the Promise is ok Actually no, because of the await. Here's the case that I was talking about:
const { setTimeout } = require('timers/promises');
async function foo() {
await setTimeout(100);
return Promise.reject(new Error('boom'));
}
async function bar() {
return foo();
}
// vs.
async function bar2() {
return await foo();
}Running bar() produces an error:
> Uncaught Error: boom
at foo (REPL33:4:23)
While running bar2() produces an error:
> Uncaught Error: boom
at foo (REPL33:4:23)
at async bar2 (REPL41:2:8)
The key is the use of the await in foo... it separates everything that comes after it into a separate stack.
There was a problem hiding this comment.
Seems like a V8 bug, interesting. Unless you feel strongly about this, I'm tempted to leave it as is until a linter rule comes to enforce it.
|
Needs a rebase. If tests are passing, then 👍 from me. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
aduh95
added
the
author ready
PR-URL: nodejs#38406 Reviewed-By: Rich Trott <[email protected]>
|
Landed in f331a18 |
PR-URL: #38406 Reviewed-By: Rich Trott <[email protected]>
|
Re. |
PR-URL: nodejs#38406 Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#38406 Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#38406 Reviewed-By: Rich Trott <[email protected]>
PR-URL: #38406 Backport-PR-URL: #39446 Reviewed-By: Rich Trott <[email protected]>
PR-URL: #38406 Backport-PR-URL: #39446 Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#38406 Backport-PR-URL: nodejs#39446 Reviewed-By: Rich Trott <[email protected]>
Refs: #38406 PR-URL: #45847 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Kohei Ueno <[email protected]>
Refs: nodejs#38406 PR-URL: nodejs#45847 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Kohei Ueno <[email protected]>
Refs: #38406 PR-URL: #45847 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Kohei Ueno <[email protected]>
Refs: #38406 PR-URL: #45847 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Kohei Ueno <[email protected]>
Refs: #38406 PR-URL: #45847 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Kohei Ueno <[email protected]>
Refs: #38406 PR-URL: #45847 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Kohei Ueno <[email protected]>
Refs: #38406 PR-URL: #45847 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Kohei Ueno <[email protected]>
This is a refactoring of the module to use
async/awaitwhere it makes more sense, and some other changes to enable ESLint on the file.The
no-restricted-syntaxrule is still disabled as the inspector throws custom errors (which we might want to include ininternal/errorsat some point), and the linter doesn't know promise versions ofsetTimeout/setInterval.