src: don't print garbage errors#4112
Conversation
|
LGTM. |
There was a problem hiding this comment.
You could simplify this to PrintErrorString("%s\n", *message ? *message : "");
I would print a message like "<toString() threw exception>" instead of a blank line.
There was a problem hiding this comment.
I would print a message like
"<toString() threw exception>"instead of a blank line.
I thought about that, but the current check is insufficient, since empty string is a valid toString() response. Would you be OK with me using a TryCatch to actually detect if an error was thrown?
There was a problem hiding this comment.
I mean when *message == nullptr, that means .toString() failed somehow.
There was a problem hiding this comment.
That doesn't seem to be the case though.
There was a problem hiding this comment.
@bnoordhuis is *message == nullptr for you? If so, it might be platform specific based on #4079 (comment).
There was a problem hiding this comment.
Ah, I missed that it's using node::Utf8Value (which indeed never returns a nullptr because it returns a pointer to its embedded char array.)
If you use String::Utf8Value, you should see *message == nullptr after a .toString() exception.
|
LGTM once @bnoordhuis is happy with it |
|
LGTM |
|
LGTM |
|
Would you mind if I land this PR? 😄 |
|
@JungMinu Don't land it yet, as the test fails on Windows. I'd also like @bnoordhuis's input. |
|
@cjihrig Sorry to bug you, It slipped my mind. 😢 |
|
IMHO, I think that using a |
If JS throws an object whose toString() method throws, then Node attempts to print an empty message, but actually prints garbage. This commit checks for this case, and prints a message instead.
|
New CI: https://ci.nodejs.org/job/node-test-pull-request/931/ Made the change to |
|
CI failures are unrelated. |
|
LGTM |
If JS throws an object whose toString() method throws, then Node attempts to print an empty message, but actually prints garbage. This commit checks for this case, and prints a message instead. Fixes: #4079 PR-URL: #4112 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
|
Thanks, landed in 1ec09b0. |
If JS throws an object whose toString() method throws, then Node attempts to print an empty message, but actually prints garbage. This commit checks for this case, and prints a message instead. Fixes: #4079 PR-URL: #4112 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
If JS throws an object whose toString() method throws, then Node attempts to print an empty message, but actually prints garbage. This commit checks for this case, and prints a message instead. Fixes: #4079 PR-URL: #4112 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
If JS throws an object whose toString() method throws, then Node attempts to print an empty message, but actually prints garbage. This commit checks for this case, and prints a message instead. Fixes: #4079 PR-URL: #4112 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
Forwarding CODER_SESSION_TOKEN via the child env exposed it to any sibling process via /proc/<pid>/environ on Linux and similar interfaces elsewhere. Drop all env injection from cliExec and instead refresh the file or keyring once via cliManager.configure inside resolveCliEnv, mirroring the connection-time write in remote.ts. The CLI reads the fresh token from the file (or keyring on supported systems) via the existing --global-config / --url flags. mTLS still works since the refresh accepts an empty token. Also drop the keyringOnly option from storeToken (no longer needed now that we always refresh) and update the matching tests. Add writeStdoutJs / writeStderrJs helpers in test/utils/platform.ts that generate fs.writeSync snippets, and use them in the cliExec and platform tests. process.stdout/stderr.write is async on POSIX pipes and can be lost on exit (nodejs/node#4112), which was making the version fallback test flaky.
Forwarding CODER_SESSION_TOKEN via the child env exposed it to any sibling process via /proc/<pid>/environ on Linux and similar interfaces elsewhere. Drop all env injection from cliExec and instead refresh the file or keyring once via cliManager.configure inside resolveCliEnv, mirroring the connection-time write in remote.ts. The CLI reads the fresh token from the file (or keyring on supported systems) via the existing --global-config / --url flags. mTLS still works since the refresh accepts an empty token. Also drop the keyringOnly option from storeToken (no longer needed now that we always refresh) and update the matching tests. Add writeStdoutJs / writeStderrJs helpers in test/utils/platform.ts that generate fs.writeSync snippets, and use them in the cliExec and platform tests. process.stdout/stderr.write is async on POSIX pipes and can be lost on exit (nodejs/node#4112), which was making the version fallback test flaky.
Forwarding CODER_SESSION_TOKEN via the child env exposed it to any sibling process via /proc/<pid>/environ on Linux and similar interfaces elsewhere. Drop all env injection from cliExec and instead refresh the file or keyring once via cliManager.configure inside resolveCliEnv, mirroring the connection-time write in remote.ts. The CLI reads the fresh token from the file (or keyring on supported systems) via the existing --global-config / --url flags. mTLS still works since the refresh accepts an empty token. Also drop the keyringOnly option from storeToken (no longer needed now that we always refresh) and update the matching tests. Add writeStdoutJs / writeStderrJs helpers in test/utils/platform.ts that generate fs.writeSync snippets, and use them in the cliExec and platform tests. process.stdout/stderr.write is async on POSIX pipes and can be lost on exit (nodejs/node#4112), which was making the version fallback test flaky.
If JS throws an object whose
toString()method throws, then Node attempts to print an empty message, but actually prints garbage. This commit checks for this case, and prints a newline instead.Closes #4079