[ty] Implement long-polling for workspace diagnsotics #19670
Conversation
MichaReiser
added
server
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
MichaReiser
force-pushed
the
micha/long-polling
branch
from
68792c3 to
cd0645c
Compare
| api::request(request); | ||
| tracing::debug!("Retrying request {}/{}", request.method, request.id); | ||
| let task = api::request(request); | ||
| scheduler.dispatch(task, &mut self.session, client); |
There was a problem hiding this comment.
Oh lol, is it that the request were never retried before this PR then?
MichaReiser
force-pushed
the
micha/diagnostic-streaming
branch
from
79178da to
2995687
Compare
MichaReiser
force-pushed
the
micha/long-polling
branch
from
317164e to
d9090a8
Compare
MichaReiser
requested review from
carljm,
AlexWaygood,
sharkdp and
dcreager
as code owners
|
crates/ty_server/src/server/api/requests/workspace_diagnostic.rs
Outdated
Show resolved
Hide resolved
crates/ty_server/src/server/api/requests/workspace_diagnostic.rs
Outdated
Show resolved
Hide resolved
crates/ty_server/src/server/api/requests/workspace_diagnostic.rs
Outdated
Show resolved
Hide resolved
| api::request(request); | ||
| tracing::debug!("Retrying request {}/{}", request.method, request.id); | ||
| let task = api::request(request); | ||
| scheduler.dispatch(task, &mut self.session, client); |
There was a problem hiding this comment.
Oh lol, is it that the request were never retried before this PR then?
|
|
||
| // Make a workspace diagnostic request to a project with one file but no diagnostics | ||
| // This should trigger long-polling since the project has no diagnostics | ||
| let request_id = send_workspace_diagnostic_request(&mut server); |
There was a problem hiding this comment.
Can we update the workspace_diagnostic_request method on TestServer with more parameters like the progress params, partial result params, etc. instead of adding another function here?
There was a problem hiding this comment.
I sort of prefer test-local helpers because the method on TestServer otherwise has to accept a WorkspaceDiagnosticParams to support all combinations, in which case it is about as verbose as calling the method directly. What I can do is to give this function a better name
There was a problem hiding this comment.
We could update the parameters on the server method to accept the parts of WorkspaceDiagnosticParams instead but I'm fine either way.
There was a problem hiding this comment.
Oh, I now remember why it is necessary. The key difference is that this method doesn't await the response because we need to handle the response specially (it's long polling)
MichaReiser
force-pushed
the
micha/diagnostic-streaming
branch
from
2995687 to
54a2995
Compare
MichaReiser
force-pushed
the
micha/long-polling
branch
from
b53424d to
c03e029
Compare
Summary
This PR implements long-polling for workspace diagnostics (as described in the spec).
Without long polling, VS Code keeps sending workspace/diagnostic requests every 2 seconds, even if the user didn't make any changes. This can take up a fair amount of CPU usage, even if all diagnostics are cached, because we still need to iterate over all files.
This PR implements long polling which avoids this. The basic idea is that the server keeps the
workspace/diagnosticopen and only responds once there are changes (e.g. new or fixed diagnostics) or the server shuts down (or cancels the request).Implementing this required adding a
processmethod to the request handler in order that workspace diagnostics can omit the response when long polling.This PR implements the last optimization mentioned in astral-sh/ty#824
Closes astral-sh/ty#824
I can no longer reproduce astral-sh/ty#818 on this PR (closes astral-sh/ty#818)
Test Plan
Added integration tests
Screen.Recording.2025-08-01.at.17.38.34.mov
Screen.Recording.2025-08-01.at.17.39.13.mov