Skip to content

Commit a61e885

Browse files
MichaReiserMichaReiser
committed
Review comments
1 parent de5ab7f commit a61e885

File tree

5 files changed

+65
-50
lines changed

5 files changed

+65
-50
lines changed

‎crates/ty_server/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ pub fn run_server() -> anyhow::Result<()> {
2727
// by default, we set the number of worker threads to `num_cpus`, with a maximum of 4.
2828
let worker_threads = std::thread::available_parallelism()
2929
.unwrap_or(four)
30-
.max(four);
30+
.min(four);
3131

3232
let (connection, io_threads) = ConnectionInitializer::stdio();
3333

‎crates/ty_server/src/server/api.rs‎

Lines changed: 27 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -122,24 +122,24 @@ pub(super) fn notification(notif: server::Notification) -> Task {
122122
})
123123
}
124124

125-
fn _local_request_task<R: traits::SyncRequestHandler>(req: server::Request) -> super::Result<Task>
125+
fn _local_request_task<R: traits::SyncRequestHandler>(req: server::Request) -> Result<Task>
126126
where
127-
<<R as RequestHandler>::RequestType as lsp_types::request::Request>::Params: UnwindSafe,
127+
<<R as RequestHandler>::RequestType as Request>::Params: UnwindSafe,
128128
{
129129
let (id, params) = cast_request::<R>(req)?;
130-
Ok(Task::local(|session, client: &Client| {
130+
Ok(Task::local(move |session, client: &Client| {
131131
let _span = tracing::debug_span!("request", %id, method = R::METHOD).entered();
132132
let result = R::run(session, client, params);
133-
respond::<R>(id, result, client);
133+
respond::<R>(&id, result, client);
134134
}))
135135
}
136136

137137
fn background_request_task<R: traits::BackgroundDocumentRequestHandler>(
138138
req: server::Request,
139139
schedule: BackgroundSchedule,
140-
) -> super::Result<Task>
140+
) -> Result<Task>
141141
where
142-
<<R as RequestHandler>::RequestType as lsp_types::request::Request>::Params: UnwindSafe,
142+
<<R as RequestHandler>::RequestType as Request>::Params: UnwindSafe,
143143
{
144144
let retry = R::RETRY_ON_CANCELLATION.then(|| req.clone());
145145
let (id, params) = cast_request::<R>(req)?;
@@ -192,7 +192,7 @@ where
192192
});
193193

194194
if let Some(response) = request_result_to_response::<R>(&id, client, result, retry) {
195-
respond::<R>(id, response, client);
195+
respond::<R>(&id, response, client);
196196
}
197197
})
198198
}))
@@ -213,7 +213,7 @@ where
213213
match result {
214214
Ok(response) => Some(response),
215215
Err(error) => {
216-
// Request was canceled due to some modifications to the salsa database.
216+
// Check if the request was canceled due to some modifications to the salsa database.
217217
if error.payload.downcast_ref::<salsa::Cancelled>().is_some() {
218218
// If the query supports retry, re-queue the request.
219219
// The query is still likely to succeed if the user modified any other document.
@@ -247,7 +247,7 @@ where
247247

248248
fn local_notification_task<N: traits::SyncNotificationHandler>(
249249
notif: server::Notification,
250-
) -> super::Result<Task> {
250+
) -> Result<Task> {
251251
let (id, params) = cast_notification::<N>(notif)?;
252252
Ok(Task::local(move |session, client| {
253253
let _span = tracing::debug_span!("notification", method = N::METHOD).entered();
@@ -262,11 +262,10 @@ fn local_notification_task<N: traits::SyncNotificationHandler>(
262262
fn background_notification_thread<N>(
263263
req: server::Notification,
264264
schedule: BackgroundSchedule,
265-
) -> super::Result<Task>
265+
) -> Result<Task>
266266
where
267267
N: traits::BackgroundDocumentNotificationHandler,
268-
<<N as NotificationHandler>::NotificationType as lsp_types::notification::Notification>::Params:
269-
UnwindSafe,
268+
<<N as NotificationHandler>::NotificationType as Notification>::Params: UnwindSafe,
270269
{
271270
let (id, params) = cast_notification::<N>(req)?;
272271
Ok(Task::background(schedule, move |session: &Session| {
@@ -309,13 +308,13 @@ where
309308
/// implementation.
310309
fn cast_request<Req>(
311310
request: server::Request,
312-
) -> super::Result<(
313-
server::RequestId,
314-
<<Req as RequestHandler>::RequestType as lsp_types::request::Request>::Params,
311+
) -> Result<(
312+
RequestId,
313+
<<Req as RequestHandler>::RequestType as Request>::Params,
315314
)>
316315
where
317-
Req: traits::RequestHandler,
318-
<<Req as RequestHandler>::RequestType as lsp_types::request::Request>::Params: UnwindSafe,
316+
Req: RequestHandler,
317+
<<Req as RequestHandler>::RequestType as Request>::Params: UnwindSafe,
319318
{
320319
request
321320
.extract(Req::METHOD)
@@ -333,26 +332,24 @@ where
333332

334333
/// Sends back a response to the server using a [`Responder`].
335334
fn respond<Req>(
336-
id: server::RequestId,
337-
result: crate::server::Result<
338-
<<Req as traits::RequestHandler>::RequestType as lsp_types::request::Request>::Result,
339-
>,
335+
id: &RequestId,
336+
result: Result<<<Req as RequestHandler>::RequestType as Request>::Result>,
340337
client: &Client,
341338
) where
342-
Req: traits::RequestHandler,
339+
Req: RequestHandler,
343340
{
344341
if let Err(err) = &result {
345342
tracing::error!("An error occurred with request ID {id}: {err}");
346343
client.show_error_message("ty encountered a problem. Check the logs for more details.");
347344
}
348-
if let Err(err) = client.respond(id, result) {
345+
if let Err(err) = client.respond(&id, result) {
349346
tracing::error!("Failed to send response: {err}");
350347
}
351348
}
352349

353350
/// Sends back an error response to the server using a [`Client`] without showing a warning
354351
/// to the user.
355-
fn respond_silent_error(id: server::RequestId, client: &Client, error: lsp_server::ResponseError) {
352+
fn respond_silent_error(id: RequestId, client: &Client, error: lsp_server::ResponseError) {
356353
if let Err(err) = client.respond_err(id, error) {
357354
tracing::error!("Failed to send response: {err}");
358355
}
@@ -362,12 +359,12 @@ fn respond_silent_error(id: server::RequestId, client: &Client, error: lsp_serve
362359
/// a parameter type for a specific request handler.
363360
fn cast_notification<N>(
364361
notification: server::Notification,
365-
) -> super::Result<
366-
(
367-
&'static str,
368-
<<N as traits::NotificationHandler>::NotificationType as lsp_types::notification::Notification>::Params,
369-
)> where
370-
N: traits::NotificationHandler,
362+
) -> Result<(
363+
&'static str,
364+
<<N as NotificationHandler>::NotificationType as Notification>::Params,
365+
)>
366+
where
367+
N: NotificationHandler,
371368
{
372369
Ok((
373370
N::METHOD,

‎crates/ty_server/src/server/api/traits.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ pub(super) trait SyncRequestHandler: RequestHandler {
2828

2929
/// A request handler that can be run on a background thread.
3030
pub(super) trait BackgroundDocumentRequestHandler: RequestHandler {
31-
/// Should this request be retried if it was cancelled due to a modification to the Salsa database?
31+
/// Whether this request be retried if it was cancelled due to a modification to the Salsa database.
3232
const RETRY_ON_CANCELLATION: bool = false;
3333

3434
fn document_url(

‎crates/ty_server/src/server/schedule/task.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ impl Task {
8383
R: Serialize + Send + 'static,
8484
{
8585
Self::local(move |_, client| {
86-
if let Err(err) = client.respond(id, result) {
86+
if let Err(err) = client.respond(&id, result) {
8787
tracing::error!("Unable to send immediate response: {err}");
8888
}
8989
})

‎crates/ty_server/src/session/client.rs‎

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -49,20 +49,28 @@ impl Client {
4949
where
5050
R: lsp_types::request::Request,
5151
{
52-
let response_handler =
53-
Box::new(move |session: &Session, response: lsp_server::Response| {
52+
let response_handler = Box::new(
53+
move |session: &Session, response: lsp_server::Response| {
54+
let _span =
55+
tracing::debug_span!("client_response", id=%response.id, method = R::METHOD)
56+
.entered();
57+
5458
match (response.error, response.result) {
5559
(Some(err), _) => {
5660
tracing::error!(
57-
"Got an error from the client (code {}): {}",
58-
err.code,
59-
err.message
61+
"Got an error from the client (code {code}, method {method}): {message}",
62+
code = err.code,
63+
message = err.message,
64+
method = R::METHOD
6065
);
6166
}
6267
(None, Some(response)) => match serde_json::from_value(response) {
6368
Ok(response) => response_handler(session, response),
6469
Err(error) => {
65-
tracing::error!("Failed to deserialize response from server: {error}");
70+
tracing::error!(
71+
"Failed to deserialize client response (method={method}): {error}",
72+
method = R::METHOD
73+
);
6674
}
6775
},
6876
(None, None) => {
@@ -75,12 +83,14 @@ impl Client {
7583
response_handler(session, serde_json::from_value(Value::Null).unwrap());
7684
} else {
7785
tracing::error!(
78-
"Server response was invalid: did not contain a result or error"
86+
"Invalid client response: did not contain a result or error (method={method})",
87+
method = R::METHOD
7988
);
8089
}
8190
}
8291
}
83-
});
92+
},
93+
);
8494

8595
let id = session
8696
.request_queue()
@@ -92,7 +102,10 @@ impl Client {
92102
id,
93103
method: R::METHOD.to_string(),
94104
params: serde_json::to_value(params).context("Failed to serialize params")?,
95-
}))?;
105+
}))
106+
.with_context(|| {
107+
format!("Failed to send request method={method}", method = R::METHOD)
108+
})?;
96109

97110
Ok(())
98111
}
@@ -108,20 +121,25 @@ impl Client {
108121
.send(lsp_server::Message::Notification(Notification::new(
109122
method, params,
110123
)))
111-
.map_err(|error| anyhow!("Failed to send notification: {error}"))
124+
.map_err(|error| {
125+
anyhow!(
126+
"Failed to send notification (method={method}): {error}",
127+
method = N::METHOD
128+
)
129+
})
112130
}
113131

114132
/// Sends a notification without any parameters to the client.
115133
///
116134
/// This is useful for notifications that don't require any data.
117135
#[expect(dead_code)]
118-
pub(crate) fn send_notification_no_params(&self, method: String) -> crate::Result<()> {
136+
pub(crate) fn send_notification_no_params(&self, method: &str) -> crate::Result<()> {
119137
self.client_sender
120138
.send(lsp_server::Message::Notification(Notification::new(
121-
method,
139+
method.to_string(),
122140
Value::Null,
123141
)))
124-
.map_err(|error| anyhow!("Failed to send notification: {error}"))
142+
.map_err(|error| anyhow!("Failed to send notification (method={method}): {error}",))
125143
}
126144

127145
/// Sends a response to the client for a given request ID.
@@ -130,22 +148,22 @@ impl Client {
130148
/// and checked for cancellation (each request must have exactly one response).
131149
pub(crate) fn respond<R>(
132150
&self,
133-
id: RequestId,
151+
id: &RequestId,
134152
result: crate::server::Result<R>,
135153
) -> crate::Result<()>
136154
where
137155
R: serde::Serialize,
138156
{
139157
let response = match result {
140-
Ok(res) => lsp_server::Response::new_ok(id, res),
158+
Ok(res) => lsp_server::Response::new_ok(id.clone(), res),
141159
Err(crate::server::Error { code, error }) => {
142-
lsp_server::Response::new_err(id, code as i32, error.to_string())
160+
lsp_server::Response::new_err(id.clone(), code as i32, error.to_string())
143161
}
144162
};
145163

146164
self.main_loop_sender
147165
.send(Event::Action(Action::SendResponse(response)))
148-
.map_err(|error| anyhow!("Failed to send response: {error}"))
166+
.map_err(|error| anyhow!("Failed to send response for request {id}: {error}"))
149167
}
150168

151169
/// Sends an error response to the client for a given request ID.

0 commit comments

Comments
 (0)