Recover from shutdown timeout in tryRestart#3602
Conversation
When the language server is hung, LanguageClient.restart() rejects because the LSP graceful shutdown exceeds vscode-languageclient's hardcoded 2-second timeout. The rejection prevents start() from running, so the user has to click Restart Server twice to actually get a new server. Catch the rejection from restart() and fall back to start(). By the time the catch runs, vscode-languageclient has already cleaned up its state, so a plain start() forks a fresh process cleanly. Fixes microsoft#3601.
andrewbranch
left a comment
There was a problem hiding this comment.
Ah, this is great, thank you! This has occasionally annoyed me but haven't had time to investigate.
There was a problem hiding this comment.
Pull request overview
This PR improves the VS Code “TypeScript Native Preview: Restart Server” command so it can recover on the first invocation when vscode-languageclient’s graceful shutdown times out due to an unresponsive/hung server.
Changes:
- Wraps
LanguageClient.restart()intry/catchwithinClient.tryRestart. - On shutdown timeout failure, logs a message and falls back to
LanguageClient.start()to force a new server process.
| catch (err) { | ||
| this.outputChannel.appendLine(`Graceful shutdown failed, forcing restart: ${err}`); | ||
| await this.client.start(); | ||
| } |
There was a problem hiding this comment.
tryRestart sets this.isInitialized = false but never sets it back to true (nor re-fires initializedEventEmitter) after a successful restart()/start(). After the first restart, extension.ts’s onLanguageServerInitialized will treat the server as uninitialized and may never invoke new listeners. Consider setting isInitialized back to true and firing the initialized event once the client has successfully started again (e.g., in a finally/post-success block).
| } | |
| } | |
| this.isInitialized = true; | |
| this.initializedEventEmitter.fire(); |
There was a problem hiding this comment.
This seems right, but not related to this PR—I'll update separately.
| this.outputChannel.appendLine(`Graceful shutdown failed, forcing restart: ${err}`); | ||
| await this.client.start(); |
There was a problem hiding this comment.
The catch will also run if LanguageClient.restart() fails for reasons other than the shutdown timeout (e.g., a failure during the subsequent start()), in which case calling start() again may mask the real error or create confusing double-start behavior. Recommend narrowing the fallback to the specific timeout failure (or rethrowing for non-timeout errors).
| this.outputChannel.appendLine(`Graceful shutdown failed, forcing restart: ${err}`); | |
| await this.client.start(); | |
| if (err instanceof Error && /timed out/i.test(err.message)) { | |
| this.outputChannel.appendLine(`Graceful shutdown failed, forcing restart: ${err}`); | |
| await this.client.start(); | |
| } | |
| else { | |
| throw err; | |
| } |
There was a problem hiding this comment.
I don't think there's any other plausible reason for restart to throw, and the client library already guards against double-starting. This is fine.
| catch (err) { | ||
| this.outputChannel.appendLine(`Graceful shutdown failed, forcing restart: ${err}`); |
There was a problem hiding this comment.
In the log line, interpolating err directly can produce unhelpful output like [object Object] and loses stack details. Consider treating it as unknown and formatting via err instanceof Error ? err.message : String(err) (and/or logging via outputChannel.error/warn to reflect that shutdown failed).
| catch (err) { | |
| this.outputChannel.appendLine(`Graceful shutdown failed, forcing restart: ${err}`); | |
| catch (err: unknown) { | |
| const message = err instanceof Error ? (err.stack ?? err.message) : String(err); | |
| this.outputChannel.error(`Graceful shutdown failed, forcing restart: ${message}`); |
There was a problem hiding this comment.
Only Error objects are thrown in this path, which stringify as expected.
Summary
Make TypeScript Native Preview: Restart Server succeed on the first click when the language server is hung.
Fixes #3601.
Problem
Client.tryRestartin_extension/src/client.tscurrently callsLanguageClient.restart()directly.LanguageClient.restart()isawait this.stop(); await this.start();.stop()performs an LSP graceful shutdown with a hardcoded 2-second timeout invscode-languageclient. When the server is hung and doesn't answershutdown/exitwithin that window,stop()rejects, the rejection bubbles up, andawait this.start()is never reached — so no new process is forked. The user sees aStopping the server timed outpopup and has to click Restart again to actually get a new server.Change
Wrap
client.restart()intry/catch. If the graceful shutdown rejects (which only happens when the server is unresponsive), fall back to a plainclient.start(). By the time the catch runs,vscode-languageclienthas already cleaned up its state toStoppedand scheduled a SIGKILL of the old process via its internalcheckProcessDied— sostart()cleanly forks a fresh server.The behavior on the happy path (server responds to
shutdownnormally) is unchanged —restart()returns successfully, the catch never runs.Verification
Manual test using the deterministic reproduction from #3601:
time.Sleep(5 * time.Second)at the top ofhandleShutdownininternal/lsp/server.go.hereby tsgo+ extension build), run extension dev host, open a TypeScript file.Before the fix:
Popup appears. Server is dead. User must click Restart a second time to get a working server.
After the fix:
No popup. Server is back up after a single click, with a ~2s delay (the unavoidable
vscode-languageclientshutdown timeout).Scope
Per CONTRIBUTING.md, only PRs addressing 6.0/7.0 differences or crashes are being accepted before TS 7.0 ships. This change is strictly client-side (TypeScript-only, ~5 lines, no Go-parity surface) and improves the recovery path from server crashes/hangs — it doesn't change any compiler behavior. Happy to defer if it's still considered out of scope, but flagging that the bug actively degrades the experience whenever a crash or hang does occur.