src: don't print garbage errors - #4112
Conversation
Sorry, something went wrong.
|
LGTM. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
I mean when *message == nullptr, that means .toString() failed somehow.
Sorry, something went wrong.
There was a problem hiding this comment.
That doesn't seem to be the case though.
Sorry, something went wrong.
There was a problem hiding this comment.
@bnoordhuis is *message == nullptr for you? If so, it might be platform specific based on #4079 (comment).
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
|
Would you mind if I land this PR? 😄 |
Sorry, something went wrong.
|
@JungMinu Don't land it yet, as the test fails on Windows. I'd also like @bnoordhuis's input. message alone, at least on OS X, doesn't seem to provide enough information to differentiate between a toString() error, and a valid empty string result. Do you think it's worth using a TryCatch to differentiate between the two? I've implemented it, and it seems very straightforward. |
Sorry, something went wrong.
|
@cjihrig Sorry to bug you, It slipped my mind. 😢 |
Sorry, something went wrong.
|
IMHO, I think that using a TryCatch to differentiate between the two is fine :) |
Sorry, something went wrong.
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 String::Utf8Value. @bnoordhuis still LGTY? |
Sorry, something went wrong.
|
CI failures are unrelated. |
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
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>
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