Repository navigation
Dispose the connection when stopping the client times out - #1937
Open
Trivikram Kamat (trivikr) wants to merge 3 commits into
Open
Trivikram Kamat (trivikr) wants to merge 3 commits into
Trivikram Kamat (trivikr) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A throwing transport end() can still skip disposal and leave pending responses and stale close handlers active, while regression coverage for timeout and rejected-shutdown cleanup is missing.
2 open issues
What changed in this PR
This PR strengthens language-client shutdown cleanup so timed-out or failed shutdowns do not leave stale connections attached to the client.
Changes:
- Moves connection ending and disposal into
finally. - Catches cleanup errors to preserve the original shutdown failure.
- Disposes the timeout timer when shutdown completes.
| File | Description |
|---|---|
client/src/common/client.ts |
Updates shutdown cleanup to handle success, failure, and timeout paths. |
Review effort: Balanced
shutdown() only disposed the connection if the shutdown finished before the timeout. On timeout or a failed shutdown request, the connection leaked and its close handler could later act on a restarted client. Dispose the connection in the finally block so it happens on every path, and clear the timeout timer.
shutdown() called connection.end() and connection.dispose() in the same try block, so if the transport's end() threw, dispose() was skipped and the error was swallowed. The connection then kept its pending responses and close handler, which is the leak the previous commit fixes. Call dispose() in an inner finally so it runs even when end() fails.
Add integration tests for a shutdown request that times out and one that the server rejects without closing the transport. Each test leaves a request pending, stops the client, and restarts it before the old server process is terminated. It then asserts that the pending request is rejected and that the old transport's late close doesn't stop the restarted client. Both tests fail without the previous fix.
Trivikram Kamat (trivikr)
force-pushed
the
client-shutdown-connection-disposal
branch
from
October 31, 2026 23:35
72be05d to
3f8880f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BaseLanguageClient.shutdown()only ended and disposed the connection when the shutdown request and exit notification finished before the timeout. If shutdown timed out or the shutdown request failed, the client dropped the connection without disposing it:handleConnectionClosed()against a restarted client and dispose its new connection.End and dispose the connection in the
finallyblock, so it happens on every path. Errors from disposing are ignored so they don't hide the original failure. Also, clear the timeout timer when shutdown finishes first.