Follow-up from the comprehensive review of PR #239 (LSP server). Line numbers reference the PR branch at review time.
Severity: major
Requests and notifications are dispatched directly on the loop thread with no std::panic::catch_unwind anywhere in apps/lsp (server.rs:205-207 for requests, server.rs:214-218 for notifications), and main.rs:36 re-panics on join (server.join().expect("the server thread panicked")). Any unwinding panic in the ide/type-checker/analysis stack (e.g. the class tracked in #240) therefore aborts the entire process.
Failure amplification: a document whose analysis panics is opened → didOpen diagnostics computation panics → process aborts → vscode-languageclient auto-restarts the server, re-sends didOpen for the same file, crashes again → after ~5 crash cycles the client stops the server permanently. One bad file converts the whole multi-document session into a permanent LSP outage, when it could be a single failed request/publish.
The README documents stack-overflow aborts as uncatchable, but ordinary panics (todo!/unwrap in analysis code) do unwind and are catchable; the code makes no distinction.
Suggested fix
- Wrap
state.handle_request(request) in std::panic::catch_unwind(AssertUnwindSafe(..)) and answer ErrorCode::InternalError (or -32803 RequestFailed) on panic.
- Guard
on_notification the same way; on panic, drop and rebuild the AnalysisHost from state.documents to avoid serving from possibly-inconsistent state.
- Keep the abort path only for genuinely unrecoverable cases (stack overflow already aborts on its own).
rust-analyzer wraps every handler in its dispatcher for exactly this reason.
Follow-up from the comprehensive review of PR #239 (LSP server). Line numbers reference the PR branch at review time.
Severity: major
Requests and notifications are dispatched directly on the loop thread with no
std::panic::catch_unwindanywhere inapps/lsp(server.rs:205-207for requests,server.rs:214-218for notifications), andmain.rs:36re-panics on join (server.join().expect("the server thread panicked")). Any unwinding panic in the ide/type-checker/analysis stack (e.g. the class tracked in #240) therefore aborts the entire process.Failure amplification: a document whose analysis panics is opened → didOpen diagnostics computation panics → process aborts → vscode-languageclient auto-restarts the server, re-sends didOpen for the same file, crashes again → after ~5 crash cycles the client stops the server permanently. One bad file converts the whole multi-document session into a permanent LSP outage, when it could be a single failed request/publish.
The README documents stack-overflow aborts as uncatchable, but ordinary panics (
todo!/unwrapin analysis code) do unwind and are catchable; the code makes no distinction.Suggested fix
state.handle_request(request)instd::panic::catch_unwind(AssertUnwindSafe(..))and answerErrorCode::InternalError(or-32803 RequestFailed) on panic.on_notificationthe same way; on panic, drop and rebuild theAnalysisHostfromstate.documentsto avoid serving from possibly-inconsistent state.rust-analyzer wraps every handler in its dispatcher for exactly this reason.