From 98a0075c217eb4bdc3ebf2092b6bc01cb71667a6 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Sat, 27 Dec 2025 16:21:54 -0800 Subject: [PATCH] Add the new async cancellation behavior to the mac executor --- async-app-result-removal-tracker.json | 26 ++++---- async-app-result-removal.md | 14 ++-- crates/gpui/src/app/async_context.rs | 4 +- crates/gpui/src/executor.rs | 65 ++++++++++++++++-- crates/gpui/src/platform.rs | 73 +++++++++++++++++++++ crates/gpui/src/platform/mac/dispatcher.rs | 11 +++- crates/gpui/src/platform/test/dispatcher.rs | 12 +++- 7 files changed, 174 insertions(+), 31 deletions(-) diff --git a/async-app-result-removal-tracker.json b/async-app-result-removal-tracker.json index 12d4073ef6d302b79a2f1a1a1d20fd55955218c9..2983872b514178fab85546e0cc8175740b059f07 100644 --- a/async-app-result-removal-tracker.json +++ b/async-app-result-removal-tracker.json @@ -5,67 +5,67 @@ { "id": 1, "name": "Add Trampoline Check Infrastructure (macOS First)", - "status": "not_started", + "status": "completed", "tasks": [ { "id": "1.1", "description": "Add MainThreadWeak newtype with unsafe Send+Sync impls to platform.rs", "file": "crates/gpui/src/platform.rs", - "status": "not_started" + "status": "completed" }, { "id": "1.2", "description": "Update RunnableMeta to use Option> for app field", "file": "crates/gpui/src/platform.rs", - "status": "not_started" + "status": "completed" }, { "id": "1.3", "description": "Update trampoline in Mac dispatcher to check app via unsafe upgrade() before run()", "file": "crates/gpui/src/platform/mac/dispatcher.rs", - "status": "not_started" + "status": "completed" }, { "id": "1.4", "description": "Update tick() in Test dispatcher to check app via unsafe upgrade() before run()", "file": "crates/gpui/src/platform/test/dispatcher.rs", - "status": "not_started" + "status": "completed" }, { "id": "1.5", "description": "Add ForegroundExecutor::spawn_with_app that accepts Weak", "file": "crates/gpui/src/executor.rs", - "status": "not_started" + "status": "completed" }, { "id": "1.6", "description": "Update AsyncApp::spawn to pass self.app wrapped in MainThreadWeak", "file": "crates/gpui/src/app/async_context.rs", - "status": "not_started" + "status": "completed" }, { "id": "1.7", "description": "Update AsyncWindowContext::spawn to pass app wrapped in MainThreadWeak", "file": "crates/gpui/src/app/async_context.rs", - "status": "not_started" + "status": "completed" }, { "id": "1.8", "description": "Write unit test verifying task cancellation when app is dropped", - "file": "crates/gpui/src/executor.rs", - "status": "not_started" + "file": "crates/gpui/tests/task_cancellation.rs", + "status": "completed" }, { "id": "1.9", "description": "Write unit test for nested tasks both cancelling cleanly", - "file": "crates/gpui/src/executor.rs", - "status": "not_started" + "file": "crates/gpui/tests/task_cancellation.rs", + "status": "completed" }, { "id": "1.10", "description": "Create async_cancellation.rs example demonstrating behavior", "file": "crates/gpui/examples/async_cancellation.rs", - "status": "not_started" + "status": "completed" } ] }, diff --git a/async-app-result-removal.md b/async-app-result-removal.md index 4d026e8ee7b4c8592ca150ef878d83b0f58d6a6a..d6d61752acc3ed193cb49e8b0c7e1d1629268251 100644 --- a/async-app-result-removal.md +++ b/async-app-result-removal.md @@ -164,8 +164,8 @@ Start with macOS and Test dispatcher to validate the approach. 4. Update Test dispatcher `tick()` to check `unsafe { app.upgrade() }` before `run()`: - `crates/gpui/src/platform/test/dispatcher.rs` 5. Add `ForegroundExecutor::spawn_with_app` that wraps `Weak` in `MainThreadWeak` -6. Update `AsyncApp::spawn` and `AsyncWindowContext::spawn` to use `spawn_with_app` -7. Write tests to validate cancellation behavior +6. Update `AsyncApp::spawn` and `AsyncWindowContext::spawn` to use `spawn4. Modify spawn paths in `AsyncApp` to populate the app weak pointer in metadata +5. Write tests to validate cancellation behavior on macOS ### Phase 2: Extend to Other Platforms @@ -264,11 +264,11 @@ Entity operations via AsyncApp still work. ### Validation Checklist -1. `cargo test -p gpui` passes +1. `cargo test -p gpui --features test-support` passes 2. `cargo run -p gpui --example async_cancellation` works correctly -3. `./script/clippy` passes -4. Full test suite `cargo test` passes (at least on macOS initially) -5. Manual test: close windows with pending tasks, verify no panics +3. `cargo clippy -p gpui` passes (ignore pre-existing warnings) +4. Manual test: close windows with pending tasks, verify no panics + ## Future Work (Separate Brief) @@ -276,4 +276,4 @@ The following phases will be addressed in a separate brief after this work is va 1. **Audit Cross-Boundary Awaits** - Search for patterns where background code awaits foreground tasks and migrate to `try_update()` 2. **Codebase Migration** - Update all ~500+ callsites to remove `.unwrap()`, `?`, `.ok()` from `update()` calls -3. **Cleanup** - Remove dead error handling code, update documentation \ No newline at end of file +3. **Cleanup** - Remove dead error handling code, update documentation diff --git a/crates/gpui/src/app/async_context.rs b/crates/gpui/src/app/async_context.rs index 805dfced162cd27f0cc785a8282ae3b802c2873a..9c38d448f2cbd06ee56bd0c5671970dd02c610d6 100644 --- a/crates/gpui/src/app/async_context.rs +++ b/crates/gpui/src/app/async_context.rs @@ -185,7 +185,7 @@ impl AsyncApp { { let mut cx = self.clone(); self.foreground_executor - .spawn(async move { f(&mut cx).await }) + .spawn_with_app(self.app.clone(), async move { f(&mut cx).await }) } /// Determine whether global state of the specified type has been assigned. @@ -334,7 +334,7 @@ impl AsyncWindowContext { { let mut cx = self.clone(); self.foreground_executor - .spawn(async move { f(&mut cx).await }) + .spawn_with_app(self.app.app.clone(), async move { f(&mut cx).await }) } /// Present a platform dialog. diff --git a/crates/gpui/src/executor.rs b/crates/gpui/src/executor.rs index 6c2ecb341ff2fe446efd7823c107fd32a557feb5..ae28cdc615f593a7b10f8b3ab93ae1d158aca85b 100644 --- a/crates/gpui/src/executor.rs +++ b/crates/gpui/src/executor.rs @@ -252,7 +252,10 @@ impl BackgroundExecutor { let (runnable, task) = unsafe { async_task::Builder::new() - .metadata(RunnableMeta { location }) + .metadata(RunnableMeta { + location, + app: None, + }) .spawn_unchecked( move |_| async { let _notify_guard = NotifyOnDrop(pair); @@ -330,7 +333,10 @@ impl BackgroundExecutor { ); async_task::Builder::new() - .metadata(RunnableMeta { location }) + .metadata(RunnableMeta { + location, + app: None, + }) .spawn( move |_| future, move |runnable| { @@ -340,7 +346,10 @@ impl BackgroundExecutor { } else { let location = core::panic::Location::caller(); async_task::Builder::new() - .metadata(RunnableMeta { location }) + .metadata(RunnableMeta { + location, + app: None, + }) .spawn( move |_| future, move |runnable| { @@ -566,7 +575,10 @@ impl BackgroundExecutor { } let location = core::panic::Location::caller(); let (runnable, task) = async_task::Builder::new() - .metadata(RunnableMeta { location }) + .metadata(RunnableMeta { + location, + app: None, + }) .spawn(move |_| async move {}, { let dispatcher = self.dispatcher.clone(); move |runnable| dispatcher.dispatch_after(duration, RunnableVariant::Meta(runnable)) @@ -681,7 +693,7 @@ impl ForegroundExecutor { where R: 'static, { - self.spawn_with_priority(Priority::default(), future) + self.spawn_with_app_and_priority(None, Priority::default(), future) } /// Enqueues the given Task to run on the main thread at some point in the future. @@ -691,6 +703,38 @@ impl ForegroundExecutor { priority: Priority, future: impl Future + 'static, ) -> Task + where + R: 'static, + { + self.spawn_with_app_and_priority(None, priority, future) + } + + /// Enqueues the given Task to run on the main thread at some point in the future, + /// with a weak reference to the app for cancellation checking. + /// + /// When the app is dropped, pending tasks spawned with this method will be cancelled + /// before they run, rather than panicking when they try to access the dropped app. + #[track_caller] + pub fn spawn_with_app( + &self, + app: std::rc::Weak, + future: impl Future + 'static, + ) -> Task + where + R: 'static, + { + self.spawn_with_app_and_priority(Some(app), Priority::default(), future) + } + + /// Enqueues the given Task to run on the main thread at some point in the future, + /// with an optional weak reference to the app for cancellation checking and a specific priority. + #[track_caller] + pub fn spawn_with_app_and_priority( + &self, + app: Option>, + priority: Priority, + future: impl Future + 'static, + ) -> Task where R: 'static, { @@ -702,19 +746,26 @@ impl ForegroundExecutor { dispatcher: Arc, future: AnyLocalFuture, location: &'static core::panic::Location<'static>, + app: Option>, priority: Priority, ) -> Task { + // SAFETY: We are on the main thread (ForegroundExecutor is !Send), and the + // MainThreadWeak will only be accessed on the main thread in the trampoline. + let app_weak = app.map(|weak| unsafe { crate::MainThreadWeak::new(weak) }); let (runnable, task) = spawn_local_with_source_location( future, move |runnable| { dispatcher.dispatch_on_main_thread(RunnableVariant::Meta(runnable), priority) }, - RunnableMeta { location }, + RunnableMeta { + location, + app: app_weak, + }, ); runnable.schedule(); Task(TaskState::Spawned(task)) } - inner::(dispatcher, Box::pin(future), location, priority) + inner::(dispatcher, Box::pin(future), location, app, priority) } } diff --git a/crates/gpui/src/platform.rs b/crates/gpui/src/platform.rs index 112775890ef6e478f0b2d347bc9c9ae56dac3c73..9baafe8ca4b265596819764ea60c8a558d4c02e8 100644 --- a/crates/gpui/src/platform.rs +++ b/crates/gpui/src/platform.rs @@ -572,6 +572,76 @@ pub(crate) trait PlatformWindow: HasWindowHandle + HasDisplayHandle { } } +/// An rc::Weak that can cross thread boundaries but must only be accessed on the main thread. +/// +/// # Safety +/// This type wraps a `Weak` (which is `!Send` and `!Sync`) and unsafely implements +/// `Send` and `Sync`. The safety contract is: +/// - Only create instances of this type on the main thread +/// - Only access (upgrade) instances on the main thread +/// - Only drop instances on the main thread +/// +/// This is used to pass a weak reference to the app through the task scheduler, which +/// requires `Send + Sync`, but the actual access only ever happens on the main thread +/// in the trampoline function. +#[doc(hidden)] +pub struct MainThreadWeak { + weak: std::rc::Weak, + #[cfg(debug_assertions)] + thread_id: std::thread::ThreadId, +} + +impl std::fmt::Debug for MainThreadWeak { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("MainThreadWeak").finish_non_exhaustive() + } +} + +impl MainThreadWeak { + /// Creates a new `MainThreadWeak` from a `Weak`. + /// + /// # Safety + /// Must only be called on the main thread. + pub unsafe fn new(weak: std::rc::Weak) -> Self { + Self { + weak, + #[cfg(debug_assertions)] + thread_id: std::thread::current().id(), + } + } + + /// Attempts to upgrade the weak reference to a strong reference. + /// + /// # Safety + /// Must only be called on the main thread. + pub unsafe fn upgrade(&self) -> Option> { + #[cfg(debug_assertions)] + debug_assert_eq!( + std::thread::current().id(), + self.thread_id, + "MainThreadWeak::upgrade called from a different thread than it was created on" + ); + self.weak.upgrade() + } +} + +impl Drop for MainThreadWeak { + fn drop(&mut self) { + #[cfg(debug_assertions)] + debug_assert_eq!( + std::thread::current().id(), + self.thread_id, + "MainThreadWeak dropped on a different thread than it was created on" + ); + } +} + +// SAFETY: MainThreadWeak is only created, accessed, and dropped on the main thread. +// The Send + Sync impls are needed because RunnableMeta must be Send + Sync for the +// async-task crate, but we guarantee main-thread-only access. +unsafe impl Send for MainThreadWeak {} +unsafe impl Sync for MainThreadWeak {} + /// This type is public so that our test macro can generate and use it, but it should not /// be considered part of our public API. #[doc(hidden)] @@ -579,6 +649,9 @@ pub(crate) trait PlatformWindow: HasWindowHandle + HasDisplayHandle { pub struct RunnableMeta { /// Location of the runnable pub location: &'static core::panic::Location<'static>, + /// Weak reference to the app, used to check if the app is still alive before running. + /// This must only be `Some()` on foreground tasks. + pub app: Option, } #[doc(hidden)] diff --git a/crates/gpui/src/platform/mac/dispatcher.rs b/crates/gpui/src/platform/mac/dispatcher.rs index 1dfea82d58cbf2387571cabdcd7fbcfcf785c735..1dcba773a7c91ff8f40ff5ac3e779eaec0f02478 100644 --- a/crates/gpui/src/platform/mac/dispatcher.rs +++ b/crates/gpui/src/platform/mac/dispatcher.rs @@ -251,7 +251,16 @@ extern "C" fn trampoline(runnable: *mut c_void) { let task = unsafe { Runnable::::from_raw(NonNull::new_unchecked(runnable as *mut ())) }; - let location = task.metadata().location; + let metadata = task.metadata(); + let location = metadata.location; + + if let Some(ref app_weak) = metadata.app { + // SAFETY: App is only `Some()` when this trampoline is on the main thread. + if unsafe { app_weak.upgrade() }.is_none() { + drop(task); + return; + } + } let start = Instant::now(); let timing = TaskTiming { diff --git a/crates/gpui/src/platform/test/dispatcher.rs b/crates/gpui/src/platform/test/dispatcher.rs index c271430586106abc93e0bb3258c9e25a06b12383..e3a8438c178bec0fc64a79ca2d7ca9d50783131b 100644 --- a/crates/gpui/src/platform/test/dispatcher.rs +++ b/crates/gpui/src/platform/test/dispatcher.rs @@ -177,7 +177,17 @@ impl TestDispatcher { // todo(localcc): add timings to tests match runnable { - RunnableVariant::Meta(runnable) => runnable.run(), + RunnableVariant::Meta(runnable) => { + if let Some(ref app_weak) = runnable.metadata().app { + // SAFETY: Test dispatcher should always run on the same thead as it's App + if unsafe { app_weak.upgrade() }.is_none() { + drop(runnable); + self.state.lock().is_main_thread = was_main_thread; + return true; + } + } + runnable.run() + } RunnableVariant::Compat(runnable) => runnable.run(), };