debugger: Fix deadlock in on_app_quit with debugger running (#29372)

Anthony Eid and Conrad Irwin created

This fixes a deadlock that would occur when `DapStore` had its on quit
handler called. The deadlock was caused by `DapStore` spawning on the
main thread while `App::shutdown` blocks the main thread.

We added a debug_panic in GPUI that panics if a foreground task is
spawned while the App context is shutting down. This will help tests
catch hangs in `cx.on_app_quit` calls.

Release Notes:

- N/A

---------

Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>

Change summary

crates/debugger_ui/src/tests/attach_modal.rs     | 12 ---
crates/debugger_ui/src/tests/console.rs          |  8 --
crates/debugger_ui/src/tests/dap_logger.rs       | 12 ---
crates/debugger_ui/src/tests/debugger_panel.rs   | 56 ------------------
crates/debugger_ui/src/tests/module_list.rs      |  8 --
crates/debugger_ui/src/tests/stack_frame_list.rs | 24 -------
crates/debugger_ui/src/tests/variable_list.rs    | 40 ------------
crates/gpui/src/app.rs                           | 15 ++++
crates/project/src/debugger/dap_store.rs         |  2 
crates/project/src/debugger/session.rs           | 11 +++
10 files changed, 26 insertions(+), 162 deletions(-)

Detailed changes

crates/debugger_ui/src/tests/attach_modal.rs 🔗

@@ -1,6 +1,6 @@
 use crate::{attach_modal::Candidate, tests::start_debug_session_with, *};
 use attach_modal::AttachModal;
-use dap::{FakeAdapter, client::SessionId};
+use dap::FakeAdapter;
 use gpui::{BackgroundExecutor, TestAppContext, VisualTestContext};
 use menu::Confirm;
 use project::{FakeFs, Project};
@@ -176,14 +176,4 @@ async fn test_show_attach_modal_and_select_process(
             assert!(workspace.active_modal::<AttachModal>(cx).is_none());
         })
         .unwrap();
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            let session = dap_store.session_by_id(SessionId(0)).unwrap();
-
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }

crates/debugger_ui/src/tests/console.rs 🔗

@@ -157,14 +157,6 @@ async fn test_handle_output_event(executor: BackgroundExecutor, cx: &mut TestApp
             );
         })
         .unwrap();
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 // #[gpui::test]

crates/debugger_ui/src/tests/dap_logger.rs 🔗

@@ -103,16 +103,4 @@ async fn test_dap_logger_captures_all_session_rpc_messages(
             hit_breakpoint_ids: None,
         }))
         .await;
-
-    cx.run_until_parked();
-
-    // Shutdown the debug session
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
-    cx.run_until_parked();
 }

crates/debugger_ui/src/tests/debugger_panel.rs 🔗

@@ -398,14 +398,6 @@ async fn test_handle_successful_run_in_terminal_reverse_request(
             assert!(running.read(cx).debug_terminal.read(cx).terminal.is_some());
         })
         .unwrap();
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 #[gpui::test]
@@ -478,14 +470,6 @@ async fn test_handle_start_debugging_request(
         .unwrap();
 
     assert_eq!(&fake_config, launched_with.lock().as_ref().unwrap());
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 // // covers that we always send a response back, if something when wrong,
@@ -556,14 +540,6 @@ async fn test_handle_error_run_in_terminal_reverse_request(
             );
         })
         .unwrap();
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 #[gpui::test]
@@ -662,14 +638,6 @@ async fn test_handle_start_debugging_reverse_request(
         send_response.load(std::sync::atomic::Ordering::SeqCst),
         "Expected to receive response from reverse request"
     );
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(child_session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 #[gpui::test]
@@ -1093,14 +1061,6 @@ async fn test_debug_panel_item_thread_status_reset_on_failure(
                 .update(cx, |session, cx| session.continue_thread(thread_id, cx));
         });
     }
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 #[gpui::test]
@@ -1256,14 +1216,6 @@ async fn test_send_breakpoints_when_editor_has_been_saved(
         called_set_breakpoints.load(std::sync::atomic::Ordering::SeqCst),
         "SetBreakpoint request must be called after editor is saved"
     );
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 #[gpui::test]
@@ -1383,14 +1335,6 @@ async fn test_unsetting_breakpoints_on_clear_breakpoint_action(
 
     cx.dispatch_action(crate::ClearAllBreakpoints);
     cx.run_until_parked();
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 #[gpui::test]

crates/debugger_ui/src/tests/module_list.rs 🔗

@@ -214,12 +214,4 @@ async fn test_module_list(executor: BackgroundExecutor, cx: &mut TestAppContext)
         assert_eq!(actual_modules.len(), 2);
         assert!(!actual_modules.contains(&changed_module));
     });
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }

crates/debugger_ui/src/tests/stack_frame_list.rs 🔗

@@ -182,14 +182,6 @@ async fn test_fetch_initial_stack_frames_and_go_to_stack_frame(
             assert_eq!(stack_frames, stack_frame_list.dap_stack_frames(cx));
         });
     });
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 #[gpui::test]
@@ -450,14 +442,6 @@ async fn test_select_stack_frame(executor: BackgroundExecutor, cx: &mut TestAppC
             })
         );
     });
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 #[gpui::test]
@@ -807,12 +791,4 @@ async fn test_collapsed_entries(executor: BackgroundExecutor, cx: &mut TestAppCo
             );
         });
     });
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }

crates/debugger_ui/src/tests/variable_list.rs 🔗

@@ -215,14 +215,6 @@ async fn test_basic_fetch_initial_scope_and_variables(
                 ]);
             });
     });
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 /// This tests fetching multiple scopes and variables for them with a single stackframe
@@ -479,14 +471,6 @@ async fn test_fetch_variables_for_multiple_scopes(
                 ]);
             });
     });
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 // tests that toggling a variable will fetch its children and shows it
@@ -1261,14 +1245,6 @@ async fn test_keyboard_navigation(executor: BackgroundExecutor, cx: &mut TestApp
                 variable_list.assert_visual_entries(vec!["> Scope 1 <=== selected", "> Scope 2"]);
             });
     });
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 #[gpui::test]
@@ -1501,14 +1477,6 @@ async fn test_variable_list_only_sends_requests_when_rendering(
         assert_eq!(frame_1_variables, variable_list.variables());
         assert!(made_scopes_request.load(Ordering::SeqCst));
     });
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }
 
 #[gpui::test]
@@ -1854,12 +1822,4 @@ async fn test_it_fetches_scopes_variables_when_you_select_a_stack_frame(
 
         assert_eq!(variables, frame_2_variables,);
     });
-
-    let shutdown_session = project.update(cx, |project, cx| {
-        project.dap_store().update(cx, |dap_store, cx| {
-            dap_store.shutdown_session(session.read(cx).session_id(), cx)
-        })
-    });
-
-    shutdown_session.await.unwrap();
 }

crates/gpui/src/app.rs 🔗

@@ -28,7 +28,7 @@ use http_client::HttpClient;
 use smallvec::SmallVec;
 #[cfg(any(test, feature = "test-support"))]
 pub use test_context::*;
-use util::ResultExt;
+use util::{ResultExt, debug_panic};
 
 use crate::{
     Action, ActionBuildError, ActionRegistry, Any, AnyView, AnyWindowHandle, AppContext, Asset,
@@ -271,6 +271,7 @@ pub struct App {
     pub(crate) tracked_entities: FxHashMap<WindowId, FxHashSet<EntityId>>,
     #[cfg(any(test, feature = "test-support", debug_assertions))]
     pub(crate) name: Option<&'static str>,
+    quitting: bool,
 }
 
 impl App {
@@ -332,6 +333,7 @@ impl App {
                 layout_id_buffer: Default::default(),
                 propagate_event: true,
                 prompt_builder: Some(PromptBuilder::Default),
+                quitting: false,
 
                 #[cfg(any(test, feature = "test-support", debug_assertions))]
                 name: None,
@@ -375,6 +377,7 @@ impl App {
         self.windows.clear();
         self.window_handles.clear();
         self.flush_effects();
+        self.quitting = true;
 
         let futures = futures::future::join_all(futures);
         if self
@@ -384,6 +387,8 @@ impl App {
         {
             log::error!("timed out waiting on app_will_quit");
         }
+
+        self.quitting = false;
     }
 
     /// Get the id of the current keyboard layout
@@ -1049,6 +1054,9 @@ impl App {
 
     /// Obtains a reference to the executor, which can be used to spawn futures.
     pub fn foreground_executor(&self) -> &ForegroundExecutor {
+        if self.quitting {
+            panic!("Can't spawn on main thread after on_app_quit")
+        };
         &self.foreground_executor
     }
 
@@ -1060,7 +1068,12 @@ impl App {
         AsyncFn: AsyncFnOnce(&mut AsyncApp) -> R + 'static,
         R: 'static,
     {
+        if self.quitting {
+            debug_panic!("Can't spawn on main thread after on_app_quit")
+        };
+
         let mut cx = self.to_async();
+
         self.foreground_executor
             .spawn(async move { f(&mut cx).await })
     }

crates/project/src/debugger/dap_store.rs 🔗

@@ -134,8 +134,6 @@ impl DapStore {
         breakpoint_store: Entity<BreakpointStore>,
         cx: &mut Context<Self>,
     ) -> Self {
-        cx.on_app_quit(Self::shutdown_sessions).detach();
-
         let locators = HashMap::from_iter([(
             "cargo".to_string(),
             Arc::new(super::locators::cargo::CargoLocator {}) as _,

crates/project/src/debugger/session.rs 🔗

@@ -700,6 +700,7 @@ impl Session {
                 BreakpointStoreEvent::ActiveDebugLineChanged => {}
             })
             .detach();
+            cx.on_app_quit(Self::on_app_quit).detach();
 
             let this = Self {
                 mode: Mode::Building,
@@ -1510,6 +1511,16 @@ impl Session {
         }
     }
 
+    fn on_app_quit(&mut self, cx: &mut Context<Self>) -> Task<()> {
+        let debug_adapter = self.adapter_client();
+
+        cx.background_spawn(async move {
+            if let Some(client) = debug_adapter {
+                client.shutdown().await.log_err();
+            }
+        })
+    }
+
     pub fn shutdown(&mut self, cx: &mut Context<Self>) -> Task<()> {
         self.is_session_terminated = true;
         self.thread_states.exit_all_threads();