[ty] Fix server hang after shutdown request #18414
Conversation
MichaReiser
added
server
|
crates/ty_server/src/server/api.rs
Outdated
| >( | ||
| req, BackgroundSchedule::LatencySensitive | ||
| ), | ||
| lsp_types::request::Shutdown::METHOD => { |
There was a problem hiding this comment.
I ended up rewriting the shutdown handling during my investigation and I sort of like this more.
There was a problem hiding this comment.
Thank you! I like this a lot, I was wondering myself whether the custom Connection struct could be removed with your recent changes around cancellation / retry.
MichaReiser
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
MichaReiser
changed the title
|
|
||
| // unregister any previously registered panic hook | ||
| // The hook will be restored when this function exits. | ||
| let _ = RestorePanicHook { |
There was a problem hiding this comment.
This is the main fix. Values assigned to _ are dropped immediately. That means, the panic hook was always restored immediately (and then overridden again).
Statements which assign an expression to an underscore causes the expression to immediately drop instead of extending the expression’s lifetime to the end of the scope. This is usually unintended, especially for types like MutexGuard, which are typically used to lock a mutex for the duration of an entire scope.
It looks like Rust 1.88 will add a lint for this source
The fix here is to assign the panic hook to a variable other than _. The Drop handler then restores the original panic hook, which in turn, drops our custom panic hook handler that holds on to the client
MichaReiser
force-pushed
the
micha/server-shutdown
branch
from
ec40828 to
8a2a4fb
Compare
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
Thank you for the quick fix! The issue seems very subtle, I think we should enable the lint rule when we can at least for the server code.
I've been also wondering whether it would make sense to invest some time to having code sharing between the Ruff and ty language server where we can. It might be possible to share the the infrastructure code i.e., the ones that involves creating the event loop.
I tested this in Neovim, the server exists successfully:
0.454553583s DEBUG ty:main ty_server::server::api: Received shutdown request, waiting for shutdown notification.
0.471809916s DEBUG ty:main ty_server::server::main_loop: Received exit notification, exiting
0.481823416s INFO main ty_server: Server shut down
I also looked at the running processes and it now only contains the server process from main.
| } | ||
|
|
||
| // Handle the response from the client to a server request | ||
| Message::Response(response) => { |
There was a problem hiding this comment.
I'm assuming we don't need to modify anything in this branch mainly because the server would stop handling any request / notification from the client when shutdown has been initiated and so the server wouldn't try to send any request to the client which means there shouldn't be any client responses to handle during the shutdown process.
There was a problem hiding this comment.
We could add the handling to notifications, because they're explicitly listed in the LSP specification. It's less clear to me if client responses are excluded too and it's quiet possible that the server might send a request from a background task when the shutdown was already initialized.
There was a problem hiding this comment.
I'll leave it as is. The LSP specification only mentions:
If a server receives requests after a shutdown request those requests should error with InvalidRequest.
Processing notifications and responses is a bit wastefull but shouldn't harm too much (they are all very short)
crates/ty_server/src/server/api.rs
Outdated
| >( | ||
| req, BackgroundSchedule::LatencySensitive | ||
| ), | ||
| lsp_types::request::Shutdown::METHOD => { |
There was a problem hiding this comment.
Thank you! I like this a lot, I was wondering myself whether the custom Connection struct could be removed with your recent changes around cancellation / retry.
It might be more work than it's worth it. The request handlers, session, etc look very different between ruff and ty |
…aration * origin/main: [ty] Treat lambda functions as instances of types.FunctionType (#18431) [ty] Fix false positives for legacy `ParamSpec`s inside `Callable` type expressions (#18426) [ty] Improve diagnostics if the user attempts to import a stdlib module that does not exist on their configured Python version (#18403) Update taiki-e/install-action action to v2.52.4 (#18420) Update docker/build-push-action action to v6.18.0 (#18422) [ty] Fix server hang after shutdown request (#18414) Update Rust crate libcst to v1.8.0 (#18424) Update Rust crate clap to v4.5.39 (#18419) Update cargo-bins/cargo-binstall action to v1.12.6 (#18416) Update dependency mdformat-mkdocs to v4.3.0 (#18421) Update pre-commit dependencies (#18418) Update dependency ruff to v0.11.12 (#18417) [ty] Ensure `Literal` types are considered assignable to anything their `Instance` supertypes are assignable to (#18351) [ty] Promote projects to good that now no longer hang (#18370) Sync vendored typeshed stubs (#18407) [ty] Fix multithreading related hangs and panics (#18238) Support relative `--ty-path` in ty-benchmark (#18385) [ty] Update docs for Python version inference (#18397) [ty] Infer the Python version from the environment if feasible (#18057) Implement template strings (#17851)
Summary
This fixes an error where the ty server didn't exit after receiving the shutdown request.
The root cause was the panic handler which hold on to a client sender channel, preventing the io thread from exiting (because there was still the chance that we would send a message).
I tested that the server now correctly exits with VS code. However, I never see an exit notification when closing VS code, only when restarting the server with the restart command. This is consistent with ruff.