Compare commits

...

7 Commits

Author SHA1 Message Date
Thorsten Ball
05e057fffd Make method only available in tests 2024-11-15 07:41:11 +01:00
Thorsten Ball
0d01cc2dd2 Make it only crate-pub 2024-11-11 08:56:13 +01:00
Thorsten Ball
e9d5c743ce Special case shutdown blocking in tests 2024-11-10 11:24:16 +01:00
Thorsten Ball
588a189fa9 Rename method 2024-11-10 10:10:43 +01:00
Thorsten Ball
60998a2922 Remove debug logging 2024-11-10 09:57:20 +01:00
Thorsten Ball
fe0cbe5cf3 Fix app shutting down only running background tasks
Co-authored-by: Antonio <antonio@zed.dev>
2024-11-10 09:48:38 +01:00
Thorsten Ball
629b8cd872 lsp: Wait for Shutdown response before sending Exit
This fixes those pesky "[lsp] oneshot canceled" error messages that
would show up when closing Zed while an LSP was running.

Bennet and I got so frustrated by them showing up in our logs that we
decided to look into it.

Long story short:

1. Spec says: "Clients should also wait with sending the `exit`
   notification until they have received a response from the `shutdown`
   request."
2. We didn't do that, we sent `exit` right away.
3. Now we wait for a response to `shutdown` before sending `exit`.

Fixes the error.

Co-authored-by: Bennet <bennet@zed.dev>
2024-11-09 18:01:02 +01:00
10 changed files with 80 additions and 35 deletions

View File

@@ -7308,6 +7308,22 @@ async fn test_document_format_manual_trigger(cx: &mut gpui::TestAppContext) {
); );
} }
#[gpui::test]
async fn test_lsp_shutdown(cx: &mut gpui::TestAppContext) {
init_test(cx, |_| {});
let cx = EditorLspTestContext::new_rust(
lsp::ServerCapabilities {
document_formatting_provider: Some(lsp::OneOf::Left(true)),
..Default::default()
},
cx,
)
.await;
drop(cx);
}
#[gpui::test] #[gpui::test]
async fn test_concurrent_format_requests(cx: &mut gpui::TestAppContext) { async fn test_concurrent_format_requests(cx: &mut gpui::TestAppContext) {
init_test(cx, |_| {}); init_test(cx, |_| {});

View File

@@ -344,7 +344,7 @@ impl AppContext {
platform.on_quit(Box::new({ platform.on_quit(Box::new({
let cx = app.clone(); let cx = app.clone();
move || { move || {
cx.borrow_mut().shutdown(); Self::shutdown(&cx);
} }
})); }));
@@ -353,23 +353,27 @@ impl AppContext {
/// Quit the application gracefully. Handlers registered with [`ModelContext::on_app_quit`] /// Quit the application gracefully. Handlers registered with [`ModelContext::on_app_quit`]
/// will be given 100ms to complete before exiting. /// will be given 100ms to complete before exiting.
pub fn shutdown(&mut self) { pub fn shutdown(this: &Rc<AppCell>) {
let mut futures = Vec::new(); let mut futures = Vec::new();
let background_executor = {
let mut this = this.borrow_mut();
for observer in self.quit_observers.remove(&()) { for observer in this.quit_observers.remove(&()) {
futures.push(observer(self)); futures.push(observer(&mut this));
} }
self.windows.clear(); this.windows.clear();
self.window_handles.clear(); this.window_handles.clear();
self.flush_effects(); this.background_executor.clone()
};
let futures = futures::future::join_all(futures); let futures = futures::future::join_all(futures);
if self #[cfg(any(test, feature = "test-support"))]
.background_executor let result = background_executor.block_to_shutdown(SHUTDOWN_TIMEOUT, futures);
.block_with_timeout(SHUTDOWN_TIMEOUT, futures) #[cfg(not(any(test, feature = "test-support")))]
.is_err() let result = background_executor.block_with_timeout(SHUTDOWN_TIMEOUT, futures);
{
if result.is_err() {
log::error!("timed out waiting on app_will_quit"); log::error!("timed out waiting on app_will_quit");
} }
} }

View File

@@ -106,6 +106,17 @@ impl Context for AsyncAppContext {
} }
impl AsyncAppContext { impl AsyncAppContext {
/// Quit the application gracefully. Handlers registered with [`ModelContext::on_app_quit`]
/// will be given 100ms to complete before exiting.
pub fn shutdown(&self) -> Result<()> {
let app = self
.app
.upgrade()
.ok_or_else(|| anyhow!("app was released"))?;
AppContext::shutdown(&app);
Ok(())
}
/// Schedules all windows in the application to be redrawn. /// Schedules all windows in the application to be redrawn.
pub fn refresh(&self) -> Result<()> { pub fn refresh(&self) -> Result<()> {
let app = self let app = self

View File

@@ -138,7 +138,7 @@ impl TestAppContext {
/// public so the macro can call it. /// public so the macro can call it.
pub fn quit(&self) { pub fn quit(&self) {
self.on_quit.borrow_mut().drain(..).for_each(|f| f()); self.on_quit.borrow_mut().drain(..).for_each(|f| f());
self.app.borrow_mut().shutdown(); AppContext::shutdown(&self.app);
} }
/// Register cleanup to run when the test ends. /// Register cleanup to run when the test ends.

View File

@@ -299,6 +299,17 @@ impl BackgroundExecutor {
self.block_internal(true, future, Some(duration)) self.block_internal(true, future, Some(duration))
} }
/// Block the current thread until the given future resolves
/// or `duration` has elapsed.
#[cfg(any(test, feature = "test-support"))]
pub(crate) fn block_to_shutdown<R>(
&self,
duration: Duration,
future: impl Future<Output = R>,
) -> Result<R, impl Future<Output = R>> {
self.block_internal(false, future, Some(duration))
}
/// Scoped lets you start a number of tasks and waits /// Scoped lets you start a number of tasks and waits
/// for all of them to complete before returning. /// for all of them to complete before returning.
pub async fn scoped<'scope, F>(&self, scheduler: F) pub async fn scoped<'scope, F>(&self, scheduler: F)

View File

@@ -161,7 +161,7 @@ fn main() {
panic!("unexpected message"); panic!("unexpected message");
} }
cx.update(|cx| cx.shutdown()).ok(); cx.shutdown().unwrap();
}) })
.detach(); .detach();
}); });

View File

@@ -23,7 +23,7 @@ pub struct LspStdoutHandler {
pub(super) notifications_channel: UnboundedReceiver<AnyNotification>, pub(super) notifications_channel: UnboundedReceiver<AnyNotification>,
} }
async fn read_headers<Stdout>(reader: &mut BufReader<Stdout>, buffer: &mut Vec<u8>) -> Result<()> async fn read_headers<Stdout>(reader: &mut BufReader<Stdout>, buffer: &mut Vec<u8>) -> Result<bool>
where where
Stdout: AsyncRead + Unpin + Send + 'static, Stdout: AsyncRead + Unpin + Send + 'static,
{ {
@@ -31,11 +31,12 @@ where
if buffer.len() >= HEADER_DELIMITER.len() if buffer.len() >= HEADER_DELIMITER.len()
&& buffer[(buffer.len() - HEADER_DELIMITER.len())..] == HEADER_DELIMITER[..] && buffer[(buffer.len() - HEADER_DELIMITER.len())..] == HEADER_DELIMITER[..]
{ {
return Ok(()); return Ok(true);
} }
// If read_until returns 0, we reached EOF and exit.
if reader.read_until(b'\n', buffer).await? == 0 { if reader.read_until(b'\n', buffer).await? == 0 {
return Err(anyhow!("cannot read LSP message headers")); return Ok(false);
} }
} }
} }
@@ -74,7 +75,9 @@ impl LspStdoutHandler {
loop { loop {
buffer.clear(); buffer.clear();
read_headers(&mut stdout, &mut buffer).await?; if !read_headers(&mut stdout, &mut buffer).await? {
return Ok(());
}
let headers = std::str::from_utf8(&buffer)?; let headers = std::str::from_utf8(&buffer)?;

View File

@@ -749,8 +749,6 @@ impl LanguageServer {
&executor, &executor,
(), (),
); );
let exit = Self::notify_internal::<notification::Exit>(&outbound_tx, ());
outbound_tx.close();
let server = self.server.clone(); let server = self.server.clone();
let name = self.name.clone(); let name = self.name.clone();
@@ -762,15 +760,16 @@ impl LanguageServer {
select! { select! {
request_result = shutdown_request.fuse() => { request_result = shutdown_request.fuse() => {
request_result?; request_result?;
let exit = Self::notify_internal::<notification::Exit>(&outbound_tx, ());
outbound_tx.close();
exit?;
} }
_ = timer => { _ = timer => {
log::info!("timeout waiting for language server {name} to shutdown"); log::info!("timeout waiting for language server {name} to shutdown");
}, },
} }
response_handlers.lock().take(); response_handlers.lock().take();
exit?;
output_done.recv().await; output_done.recv().await;
server.lock().take().map(|mut child| child.kill()); server.lock().take().map(|mut child| child.kill());
log::debug!("language server shutdown finished"); log::debug!("language server shutdown finished");
@@ -1238,6 +1237,8 @@ impl FakeLanguageServer {
} }
}); });
fake.handle_request::<request::Shutdown, _, _>(|_, _| async move { Ok(()) });
(server, fake) (server, fake)
} }

View File

@@ -24,7 +24,7 @@ use std::{
path::{Path, PathBuf}, path::{Path, PathBuf},
sync::{atomic::AtomicUsize, Arc}, sync::{atomic::AtomicUsize, Arc},
}; };
use util::ResultExt; use util::{ResultExt, TryFutureExt};
use worktree::Worktree; use worktree::Worktree;
pub struct HeadlessProject { pub struct HeadlessProject {
@@ -554,13 +554,14 @@ impl HeadlessProject {
_envelope: TypedEnvelope<proto::ShutdownRemoteServer>, _envelope: TypedEnvelope<proto::ShutdownRemoteServer>,
cx: AsyncAppContext, cx: AsyncAppContext,
) -> Result<proto::Ack> { ) -> Result<proto::Ack> {
cx.spawn(|cx| async move { cx.spawn(|cx| {
cx.update(|cx| { async move {
// TODO: This is a hack, because in a headless project, shutdown isn't executed // TODO: This is a hack, because in a headless project, shutdown isn't executed
// when calling quit, but it should be. // when calling quit, but it should be.
cx.shutdown(); AsyncAppContext::shutdown(&cx)?;
cx.quit(); cx.update(|cx| cx.quit())
}) }
.log_err()
}) })
.detach(); .detach();

View File

@@ -284,12 +284,10 @@ fn start_server(
} }
_ = futures::FutureExt::fuse(smol::Timer::after(IDLE_TIMEOUT)) => { _ = futures::FutureExt::fuse(smol::Timer::after(IDLE_TIMEOUT)) => {
log::warn!("timed out waiting for new connections after {:?}. exiting.", IDLE_TIMEOUT); log::warn!("timed out waiting for new connections after {:?}. exiting.", IDLE_TIMEOUT);
cx.update(|cx| { // TODO: This is a hack, because in a headless project, shutdown isn't executed
// TODO: This is a hack, because in a headless project, shutdown isn't executed // when calling quit, but it should be.
// when calling quit, but it should be. cx.shutdown()?;
cx.shutdown(); cx.update(|cx| cx.quit())?;
cx.quit();
})?;
break; break;
} }
_ = app_quit_rx.next().fuse() => { _ = app_quit_rx.next().fuse() => {