Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

debugger: Fix shutdown issues (#27071)

Browse source 
This PR fixes a few issues around shutting down a debug adapter.

The first issue I discovered was when I shut down all sessions via
`shutdown all adapters` command. We would still fetch the threads
request again, because we receive a thread event that indicated that it
exited. But this will always time out because the debug adapter is
already shutdown at this point, so by updating the check so we don't
allow fetching a request when the session is terminated fixes the issue.

The second issue fixes a bug where we would always shut down the parent
session, when a child session is terminated. This was reintroduced by
the big refactor. This is not something we want, because you could
receive multiple StartDebugging reverse requests, so if one child is
shutting down that does not mean the other ones should have been
shutting down as well.
Issue was original fixed in
https://github.com/RemcoSmitsDev/zed/pull/80#issuecomment-2573943661.


## TODO:
- [x] Add tests

Release Notes:

- N/A
This commit is contained in:
Remco Smits 2025-03-20 19:32:37 +01:00 • committed by GitHub
parent 7b80cd865d
commit ac452799b0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 287 additions and 6 deletions
Download patch file Download diff file Expand all files Collapse all files

226
crates/debugger_ui/src/tests/debugger_panel.rs
View file

@ -602,7 +602,7 @@ async fn test_handle_start_debugging_reverse_request(
});
let child_client = child_session.update(cx, |session, _| session.adapter_client().unwrap());
client
child_client
.on_request::<dap::requests::Threads, _>(move |_, _| {
Ok(dap::ThreadsResponse {
threads: vec![dap::Thread {
@ -645,6 +645,230 @@ async fn test_handle_start_debugging_reverse_request(
shutdown_session.await.unwrap();
}
#[gpui::test]
async fn test_shutdown_children_when_parent_session_shutdown(
executor: BackgroundExecutor,
cx: &mut TestAppContext,
) {
init_test(cx);
let fs = FakeFs::new(executor.clone());
fs.insert_tree(
"/project",
json!({
"main.rs": "First line\nSecond line\nThird line\nFourth line",
}),
)
.await;
let project = Project::test(fs, ["/project".as_ref()], cx).await;
let dap_store = project.update(cx, |project, _| project.dap_store());
let workspace = init_test_workspace(&project, cx).await;
let cx = &mut VisualTestContext::from_window(*workspace, cx);
let task = project.update(cx, |project, cx| {
project.start_debug_session(dap::test_config(DebugRequestType::Launch, None, None), cx)
});
let parent_session = task.await.unwrap();
let client = parent_session.update(cx, |session, _| session.adapter_client().unwrap());
client
.on_request::<dap::requests::Threads, _>(move |_, _| {
Ok(dap::ThreadsResponse {
threads: vec![dap::Thread {
id: 1,
name: "Thread 1".into(),
}],
})
})
.await;
client.on_response::<StartDebugging, _>(move |_| {}).await;
// start first child session
client
.fake_reverse_request::<StartDebugging>(StartDebuggingRequestArguments {
configuration: json!({}),
request: StartDebuggingRequestArgumentsRequest::Launch,
})
.await;
cx.run_until_parked();
// start second child session
client
.fake_reverse_request::<StartDebugging>(StartDebuggingRequestArguments {
configuration: json!({}),
request: StartDebuggingRequestArgumentsRequest::Launch,
})
.await;
cx.run_until_parked();
// configure first child session
let first_child_session = dap_store.read_with(cx, |dap_store, _| {
dap_store.session_by_id(SessionId(1)).unwrap()
});
let first_child_client =
first_child_session.update(cx, |session, _| session.adapter_client().unwrap());
first_child_client
.on_request::<Disconnect, _>(move |_, _| Ok(()))
.await;
// configure second child session
let second_child_session = dap_store.read_with(cx, |dap_store, _| {
dap_store.session_by_id(SessionId(2)).unwrap()
});
let second_child_client =
second_child_session.update(cx, |session, _| session.adapter_client().unwrap());
second_child_client
.on_request::<Disconnect, _>(move |_, _| Ok(()))
.await;
cx.run_until_parked();
// shutdown parent session
dap_store
.update(cx, |dap_store, cx| {
dap_store.shutdown_session(parent_session.read(cx).session_id(), cx)
})
.await
.unwrap();
// assert parent session and all children sessions are shutdown
dap_store.update(cx, |dap_store, cx| {
assert!(dap_store
.session_by_id(parent_session.read(cx).session_id())
.is_none());
assert!(dap_store
.session_by_id(first_child_session.read(cx).session_id())
.is_none());
assert!(dap_store
.session_by_id(second_child_session.read(cx).session_id())
.is_none());
});
}
#[gpui::test]
async fn test_shutdown_parent_session_if_all_children_are_shutdown(
executor: BackgroundExecutor,
cx: &mut TestAppContext,
) {
init_test(cx);
let fs = FakeFs::new(executor.clone());
fs.insert_tree(
"/project",
json!({
"main.rs": "First line\nSecond line\nThird line\nFourth line",
}),
)
.await;
let project = Project::test(fs, ["/project".as_ref()], cx).await;
let dap_store = project.update(cx, |project, _| project.dap_store());
let workspace = init_test_workspace(&project, cx).await;
let cx = &mut VisualTestContext::from_window(*workspace, cx);
let task = project.update(cx, |project, cx| {
project.start_debug_session(dap::test_config(DebugRequestType::Launch, None, None), cx)
});
let parent_session = task.await.unwrap();
let client = parent_session.update(cx, |session, _| session.adapter_client().unwrap());
client.on_response::<StartDebugging, _>(move |_| {}).await;
// start first child session
client
.fake_reverse_request::<StartDebugging>(StartDebuggingRequestArguments {
configuration: json!({}),
request: StartDebuggingRequestArgumentsRequest::Launch,
})
.await;
cx.run_until_parked();
// start second child session
client
.fake_reverse_request::<StartDebugging>(StartDebuggingRequestArguments {
configuration: json!({}),
request: StartDebuggingRequestArgumentsRequest::Launch,
})
.await;
cx.run_until_parked();
// configure first child session
let first_child_session = dap_store.read_with(cx, |dap_store, _| {
dap_store.session_by_id(SessionId(1)).unwrap()
});
let first_child_client =
first_child_session.update(cx, |session, _| session.adapter_client().unwrap());
first_child_client
.on_request::<Disconnect, _>(move |_, _| Ok(()))
.await;
// configure second child session
let second_child_session = dap_store.read_with(cx, |dap_store, _| {
dap_store.session_by_id(SessionId(2)).unwrap()
});
let second_child_client =
second_child_session.update(cx, |session, _| session.adapter_client().unwrap());
second_child_client
.on_request::<Disconnect, _>(move |_, _| Ok(()))
.await;
cx.run_until_parked();
// shutdown first child session
dap_store
.update(cx, |dap_store, cx| {
dap_store.shutdown_session(first_child_session.read(cx).session_id(), cx)
})
.await
.unwrap();
// assert parent session and second child session still exist
dap_store.update(cx, |dap_store, cx| {
assert!(dap_store
.session_by_id(parent_session.read(cx).session_id())
.is_some());
assert!(dap_store
.session_by_id(first_child_session.read(cx).session_id())
.is_none());
assert!(dap_store
.session_by_id(second_child_session.read(cx).session_id())
.is_some());
});
// shutdown first child session
dap_store
.update(cx, |dap_store, cx| {
dap_store.shutdown_session(second_child_session.read(cx).session_id(), cx)
})
.await
.unwrap();
// assert parent session got shutdown by second child session
// because it was the last child
dap_store.update(cx, |dap_store, cx| {
assert!(dap_store
.session_by_id(parent_session.read(cx).session_id())
.is_none());
assert!(dap_store
.session_by_id(second_child_session.read(cx).session_id())
.is_none());
});
}
#[gpui::test]
async fn test_debug_panel_item_thread_status_reset_on_failure(
executor: BackgroundExecutor,

39
crates/project/src/debugger/dap_store.rs
View file

@ -28,7 +28,7 @@ use dap::{
use fs::Fs;
use futures::{
channel::{mpsc, oneshot},
future::Shared,
future::{join_all, Shared},
};
use gpui::{App, AppContext, AsyncApp, Context, Entity, EventEmitter, SharedString, Task};
use http_client::HttpClient;
@ -348,6 +348,12 @@ impl DapStore {
);
let session_id = local_store.next_session_id();
if let Some(session) = &parent_session {
session.update(cx, |session, _| {
session.add_child_session_id(session_id);
});
}
let (initialized_tx, initialized_rx) = oneshot::channel();
let start_client_task = Session::local(
@ -764,13 +770,40 @@ impl DapStore {
return Task::ready(Err(anyhow!("Could not find session: {:?}", session_id)));
};
let shutdown_parent_task = session
let shutdown_children = session
.read(cx)
.child_session_ids()
.iter()
.map(|session_id| self.shutdown_session(*session_id, cx))
.collect::<Vec<_>>();
let shutdown_parent_task = if let Some(parent_session) = session
.read(cx)
.parent_id()
.map(|parent_id| self.shutdown_session(parent_id, cx));
.and_then(|session_id| self.session_by_id(session_id))
{
let shutdown_id = parent_session.update(cx, |parent_session, _| {
parent_session.remove_child_session_id(session_id);
if parent_session.child_session_ids().len() == 0 {
Some(parent_session.session_id())
} else {
None
}
});
shutdown_id.map(|session_id| self.shutdown_session(session_id, cx))
} else {
None
};
let shutdown_task = session.update(cx, |this, cx| this.shutdown(cx));
cx.background_spawn(async move {
if shutdown_children.len() > 0 {
let _ = join_all(shutdown_children).await;
}
shutdown_task.await;
if let Some(parent_task) = shutdown_parent_task {

28
crates/project/src/debugger/session.rs
View file

@ -11,7 +11,7 @@ use super::dap_command::{
};
use super::dap_store::DapAdapterDelegate;
use anyhow::{anyhow, Result};
use collections::{HashMap, IndexMap, IndexSet};
use collections::{HashMap, HashSet, IndexMap, IndexSet};
use dap::adapters::{DebugAdapter, DebugAdapterBinary};
use dap::messages::Response;
use dap::OutputEventCategory;
@ -522,6 +522,11 @@ impl ThreadStates {
self.known_thread_states.clear();
}
fn exit_all_threads(&mut self) {
self.global_state = Some(ThreadStatus::Exited);
self.known_thread_states.clear();
}
fn continue_all_threads(&mut self) {
self.global_state = Some(ThreadStatus::Running);
self.known_thread_states.clear();
@ -577,6 +582,7 @@ pub struct Session {
mode: Mode,
pub(super) capabilities: Capabilities,
id: SessionId,
child_session_ids: HashSet<SessionId>,
parent_id: Option<SessionId>,
ignore_breakpoints: bool,
modules: Vec<dap::Module>,
@ -753,6 +759,7 @@ impl Session {
Self {
mode: Mode::Local(mode),
id: session_id,
child_session_ids: HashSet::default(),
parent_id: parent_session.map(|session| session.read(cx).id),
variables: Default::default(),
capabilities,
@ -785,13 +792,13 @@ impl Session {
_upstream_project_id: upstream_project_id,
}),
id: session_id,
child_session_ids: HashSet::default(),
parent_id: None,
capabilities: Capabilities::default(),
ignore_breakpoints,
variables: Default::default(),
stack_frames: Default::default(),
thread_states: ThreadStates::default(),
output_token: OutputToken(0),
output: circular_buffer::CircularBuffer::boxed(),
requests: HashMap::default(),
@ -808,6 +815,18 @@ impl Session {
self.id
}
pub fn child_session_ids(&self) -> HashSet<SessionId> {
self.child_session_ids.clone()
}
pub fn add_child_session_id(&mut self, session_id: SessionId) {
self.child_session_ids.insert(session_id);
}
pub fn remove_child_session_id(&mut self, session_id: SessionId) {
self.child_session_ids.remove(&session_id);
}
pub fn parent_id(&self) -> Option<SessionId> {
self.parent_id
}
@ -1051,6 +1070,7 @@ impl Session {
if !self.thread_states.any_stopped_thread()
&& request.type_id() != TypeId::of::<ThreadsCommand>()
|| self.is_session_terminated
{
return;
}
@ -1331,6 +1351,10 @@ impl Session {
}
pub fn shutdown(&mut self, cx: &mut Context<Self>) -> Task<()> {
self.is_session_terminated = true;
self.thread_states.exit_all_threads();
cx.notify();
let task = if self
.capabilities
.supports_terminate_request