Conversation
| void showAndLogExceptionWithTelemetry( | ||
| redactableError(asError(error))`Unhandled error: ${getErrorMessage( | ||
| error, | ||
| )}`, | ||
| ); |
There was a problem hiding this comment.
What happens if showAndLogExceptionWithTelemetry throws? It will be an unhandledRejection, I think. And this would cause a loop of this handler being called. Unlikely to happen, but if it happens, it will be bad.
I think it would be best to add a .then(...) handler here, with a catch argument that just logs to. Or, you can just be really sure that showAndLogExceptionWithTelemetry will never throw.
There was a problem hiding this comment.
Good point. I think we can add a catch to showAndLogExceptionWithTelemetry which then logs in a much more simple way. Or alternatively we could just fully ignore it in this case. That wouldn't put us in a worse place than we were before this PR.
60532e1 to
5e5834e
Compare
5e5834e to
2ebfea1
Compare
| // triggering "unhandledRejection" and avoid an infinite loop | ||
| showAndLogExceptionWithTelemetry(message).catch( | ||
| (telemetryError: unknown) => { | ||
| void extLogger.log( |
There was a problem hiding this comment.
To test this I tried throwing a test exception, and also added an error to showAndLogExceptionWithTelemetry. Annoying something weird is going on and the second error seems to be getting lost. For example it prints
Failed to send error telemetry: Cannot read properties of undefined (reading 't')
Unhandled error: Cannot read properties of undefined (reading 't')
even though the two messages should be different.
I'm therefore not sure whether to keep this or just log one of the messages.
|
I've done some more manual testing of having or not having the rejection handler and throwing exceptions in various ways. I think it's all working as expected and won't cause problems. Also no telemetry is sent unless the config option is enabled, and that option is still currently off by default. It does slightly change behaviour because some exceptions were previously lost and not shown to the user. If an exception happens in a command then it'll be shown to the user as an error, with the command name. If an exception happens within something like a I think this change of behaviour is acceptable. The existing logging is done by us, so our intention is to log exceptions. Therefore this PR makes the situation more consistent because now we're logging more exceptions that previously were lost. |
Adds listeners for unhandled exceptions or rejected promises. The intention here is to emit telemetry for them so that we know how frequently this is happening for users.
There is some general advice online that one should not just catch unhandled exceptions and try to carry on regardless. I think this PR isn't making that behaviour any more unsafe because that's already how VS Code behaves for exceptions in extension code. We already aren't crashing the whole NodeJS process, so I think by adding this handler to logs telemetry we aren't changing the overall program behaviour.
Checklist
ready-for-doc-reviewlabel there.