Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_cli): improve the logging of panics and connection errors #3957

Merged
merged 3 commits into from
Dec 5, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Dec 5, 2022

Summary

Fixes #3920

This is a followup to #3952 adding additional logs to the CLI and VSCode extension. Specifically, it extends the panic hook of the CLI to print the panic message to the current tracing log file in addition to stderr. This ensures that panic messages can be retrieved in the logs when they cause a background process like the language server to crash.

In addition to this, I extended the previous changes to the VSCode extension to wrap errors thrown by the connect function in addition to the exceptions raised by the socket itself. I also improved the getSocket function by collecting the content of the stderr pipe for the CLI child process in addition to stdout, and to fail with an error if the content of stdout was empty. This error will now include the exit code of the CLI as well as the content of stderr if it was not empty.

Test Plan

I tested these changes locally by introducing explicit panics in the print_socket and run_daemon functions to ensure that the panic is correctly getting logged to the tracing log file, and that the extension correctly prints the content of stderr if the CLI instance exits with an error.

@leops leops requested review from ematipico and a team as code owners December 5, 2022 15:36
@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit f8e7535
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/638e20d47f036100082b192d

editors/vscode/src/main.ts Outdated Show resolved Hide resolved
@leops
Copy link
Contributor Author

leops commented Dec 5, 2022

I've updated the child process handling code in the extension to wait for the stdout and stderr streams to be finished in addition to the process having exited before reading the content of the corresponding buffers

@MichaReiser
Copy link
Contributor

Fixes #3920

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants