From 626bd86e6206eb9ffaa1b52930bc8e0c32a7d019 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Sat, 27 Dec 2025 22:26:58 -0800 Subject: [PATCH] Add tests for the basic executor cases --- async-app-result-removal-tracker.json | 9 +- crates/gpui/src/executor.rs | 334 ++++++++++++++++++++++++++ 2 files changed, 339 insertions(+), 4 deletions(-) diff --git a/async-app-result-removal-tracker.json b/async-app-result-removal-tracker.json index 2983872b514178fab85546e0cc8175740b059f07..82c296f4e3d741ea25b64abfa310d8c2482bbf2d 100644 --- a/async-app-result-removal-tracker.json +++ b/async-app-result-removal-tracker.json @@ -51,14 +51,15 @@ }, { "id": "1.8", - "description": "Write unit test verifying task cancellation when app is dropped", - "file": "crates/gpui/tests/task_cancellation.rs", - "status": "completed" + "description": "Write unit tests verifying task cancellation when app is dropped (moved to executor.rs to access pub(crate) items)", + "file": "crates/gpui/src/executor.rs", + "status": "completed", + "notes": "Tests moved from tests/task_cancellation.rs to executor.rs as #[cfg(test)] module because TestAppContext holds strong Rc preventing proper app-drop testing" }, { "id": "1.9", "description": "Write unit test for nested tasks both cancelling cleanly", - "file": "crates/gpui/tests/task_cancellation.rs", + "file": "crates/gpui/src/executor.rs", "status": "completed" }, { diff --git a/crates/gpui/src/executor.rs b/crates/gpui/src/executor.rs index ae28cdc615f593a7b10f8b3ab93ae1d158aca85b..3b036af6577ad7f7f451c42e456b33c1065680c1 100644 --- a/crates/gpui/src/executor.rs +++ b/crates/gpui/src/executor.rs @@ -125,6 +125,30 @@ impl Task { Task(TaskState::Spawned(task)) => task.detach(), } } + + /// Converts this task into a fallible task that returns `Option`. + /// + /// Unlike the standard `Task`, a [`FallibleTask`] will return `None` + /// if the app was dropped while the task is executing. + /// + /// # Example + /// + /// ```ignore + /// // Background task that gracefully handles app shutdown: + /// cx.background_spawn(async move { + /// let result = foreground_task.fallible().await; + /// if let Some(value) = result { + /// // Process the value + /// } + /// // If None, app was shut down - just exit gracefully + /// }).detach(); + /// ``` + pub fn fallible(self) -> FallibleTask { + FallibleTask(match self.0 { + TaskState::Ready(val) => FallibleTaskState::Ready(val), + TaskState::Spawned(task) => FallibleTaskState::Spawned(task.fallible()), + }) + } } impl Task> @@ -154,6 +178,55 @@ impl Future for Task { } } +/// A task that returns `Option` instead of panicking when cancelled. +#[must_use] +pub struct FallibleTask(FallibleTaskState); + +enum FallibleTaskState { + /// A task that is ready to return a value + Ready(Option), + + /// A task that is currently running (wraps async_task::FallibleTask). + Spawned(async_task::FallibleTask), +} + +impl FallibleTask { + /// Creates a new fallible task that will resolve with the value. + pub fn ready(val: T) -> Self { + FallibleTask(FallibleTaskState::Ready(Some(val))) + } + + /// Detaching a task runs it to completion in the background. + pub fn detach(self) { + match self.0 { + FallibleTaskState::Ready(_) => {} + FallibleTaskState::Spawned(task) => task.detach(), + } + } +} + +impl Future for FallibleTask { + type Output = Option; + + fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll { + match unsafe { self.get_unchecked_mut() } { + FallibleTask(FallibleTaskState::Ready(val)) => Poll::Ready(val.take()), + FallibleTask(FallibleTaskState::Spawned(task)) => Pin::new(task).poll(cx), + } + } +} + +impl std::fmt::Debug for FallibleTask { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match &self.0 { + FallibleTaskState::Ready(_) => f.debug_tuple("FallibleTask::Ready").finish(), + FallibleTaskState::Spawned(task) => { + f.debug_tuple("FallibleTask::Spawned").field(task).finish() + } + } + } +} + /// A task label is an opaque identifier that you can use to /// refer to a task in tests. #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)] @@ -898,3 +971,264 @@ impl Drop for Scope<'_> { self.executor.block(self.rx.next()); } } + +#[cfg(test)] +mod test { + use super::*; + use crate::{App, TestDispatcher, TestPlatform}; + use rand::SeedableRng; + use std::{cell::RefCell, rc::Weak}; + + #[test] + fn sanity_test_tasks_run() { + let dispatcher = TestDispatcher::new(StdRng::seed_from_u64(0)); + let arc_dispatcher = Arc::new(dispatcher.clone()); + let background_executor = BackgroundExecutor::new(arc_dispatcher.clone()); + let foreground_executor = ForegroundExecutor::new(arc_dispatcher); + + let platform = TestPlatform::new(background_executor, foreground_executor.clone()); + let asset_source = Arc::new(()); + let http_client = http_client::FakeHttpClient::with_404_response(); + + let app = App::new_app(platform, asset_source, http_client); + + let task_ran = Rc::new(RefCell::new(false)); + + foreground_executor + .spawn_with_app(Rc::downgrade(&app), { + let task_ran = Rc::clone(&task_ran); + async move { + *task_ran.borrow_mut() = true; + } + }) + .detach(); + + // Run dispatcher while app is still alive + dispatcher.run_until_parked(); + + // Task should have run + assert!( + *task_ran.borrow(), + "Task should run normally when app is alive" + ); + } + + #[test] + fn test_task_cancelled_when_app_dropped() { + let dispatcher = TestDispatcher::new(StdRng::seed_from_u64(0)); + let arc_dispatcher = Arc::new(dispatcher.clone()); + let background_executor = BackgroundExecutor::new(arc_dispatcher.clone()); + let foreground_executor = ForegroundExecutor::new(arc_dispatcher); + + let platform = TestPlatform::new(background_executor, foreground_executor.clone()); + let asset_source = Arc::new(()); + let http_client = http_client::FakeHttpClient::with_404_response(); + + let app = App::new_app(platform, asset_source, http_client); + let app_weak = Rc::downgrade(&app); + + let task_ran = Rc::new(RefCell::new(false)); + let task_ran_clone = Rc::clone(&task_ran); + + foreground_executor + .spawn_with_app(Weak::clone(&app_weak), async move { + *task_ran_clone.borrow_mut() = true; + }) + .detach(); + + assert!( + Rc::weak_count(&app) > 0, + "Task should hold a weak reference" + ); + + drop(app); + + assert!(app_weak.upgrade().is_none(), "App should have been dropped"); + + dispatcher.run_until_parked(); + + // The task should have been cancelled, not run + assert!( + !*task_ran.borrow(), + "Task should have been cancelled when app was dropped, but it ran!" + ); + } + + #[test] + fn test_nested_tasks_both_cancel() { + let dispatcher = TestDispatcher::new(StdRng::seed_from_u64(0)); + let arc_dispatcher = Arc::new(dispatcher.clone()); + let background_executor = BackgroundExecutor::new(arc_dispatcher.clone()); + let foreground_executor = ForegroundExecutor::new(arc_dispatcher); + + let platform = TestPlatform::new(background_executor, foreground_executor.clone()); + let asset_source = Arc::new(()); + let http_client = http_client::FakeHttpClient::with_404_response(); + + let app = App::new_app(platform, asset_source, http_client); + let app_weak = Rc::downgrade(&app); + + let outer_completed = Rc::new(RefCell::new(false)); + let inner_completed = Rc::new(RefCell::new(false)); + let reached_await = Rc::new(RefCell::new(false)); + + let outer_flag = Rc::clone(&outer_completed); + let inner_flag = Rc::clone(&inner_completed); + let await_flag = Rc::clone(&reached_await); + + // Channel to block the inner task until we're ready + let (tx, rx) = futures::channel::oneshot::channel::<()>(); + + // We need clones of executor and app_weak for the inner spawn + let inner_executor = foreground_executor.clone(); + let inner_app_weak = app_weak.clone(); + + // Spawn outer task that will spawn and await an inner task + foreground_executor + .spawn_with_app(Weak::clone(&app_weak), async move { + let inner_flag_clone = Rc::clone(&inner_flag); + + // Spawn inner task that blocks on a channel + let inner_task = inner_executor.spawn_with_app(inner_app_weak, async move { + // Wait for signal (which will never come - we'll drop the app instead) + rx.await.ok(); + *inner_flag_clone.borrow_mut() = true; + }); + + // Mark that we've reached the await point + *await_flag.borrow_mut() = true; + + // Await inner task - this should not panic when both are cancelled + inner_task.await; + + // Mark outer as complete (should never reach here) + *outer_flag.borrow_mut() = true; + }) + .detach(); + + // Run dispatcher until outer task reaches the await point + // The inner task will be blocked on the channel + dispatcher.run_until_parked(); + + // Verify we actually reached the await point before dropping the app + assert!( + *reached_await.borrow(), + "Outer task should have reached the await point" + ); + + // Neither task should have completed yet + assert!( + !*outer_completed.borrow(), + "Outer task should not have completed yet" + ); + assert!( + !*inner_completed.borrow(), + "Inner task should not have completed yet" + ); + + // Drop the channel sender and app while outer is awaiting inner + drop(tx); + drop(app); + assert!(app_weak.upgrade().is_none(), "App should have been dropped"); + + // Run dispatcher - both tasks should be cancelled + dispatcher.run_until_parked(); + + // Neither task should have completed (both were cancelled) + assert!( + !*outer_completed.borrow(), + "Outer task should have been cancelled, not completed" + ); + assert!( + !*inner_completed.borrow(), + "Inner task should have been cancelled, not completed" + ); + } + + #[test] + fn test_task_without_app_tracking_still_runs() { + let dispatcher = TestDispatcher::new(StdRng::seed_from_u64(0)); + let arc_dispatcher = Arc::new(dispatcher.clone()); + let background_executor = BackgroundExecutor::new(arc_dispatcher.clone()); + let foreground_executor = ForegroundExecutor::new(arc_dispatcher); + + let platform = TestPlatform::new(background_executor, foreground_executor.clone()); + let asset_source = Arc::new(()); + let http_client = http_client::FakeHttpClient::with_404_response(); + + let app = App::new_app(platform, asset_source, http_client); + let app_weak = Rc::downgrade(&app); + + let task_ran = Rc::new(RefCell::new(false)); + let task_ran_clone = Rc::clone(&task_ran); + + let _task = foreground_executor.spawn(async move { + *task_ran_clone.borrow_mut() = true; + }); + + drop(app); + + assert!(app_weak.upgrade().is_none(), "App should have been dropped"); + + dispatcher.run_until_parked(); + + assert!( + *task_ran.borrow(), + "Task without app tracking should still run after app is dropped" + ); + } + + #[test] + #[should_panic] + fn test_polling_cancelled_task_panics() { + let dispatcher = TestDispatcher::new(StdRng::seed_from_u64(0)); + let arc_dispatcher = Arc::new(dispatcher.clone()); + let background_executor = BackgroundExecutor::new(arc_dispatcher.clone()); + let foreground_executor = ForegroundExecutor::new(arc_dispatcher); + + let platform = TestPlatform::new(background_executor.clone(), foreground_executor.clone()); + let asset_source = Arc::new(()); + let http_client = http_client::FakeHttpClient::with_404_response(); + + let app = App::new_app(platform, asset_source, http_client); + let app_weak = Rc::downgrade(&app); + + let task = foreground_executor.spawn_with_app(Weak::clone(&app_weak), async move { 42 }); + + drop(app); + + assert!(app_weak.upgrade().is_none(), "App should have been dropped"); + + dispatcher.run_until_parked(); + + background_executor.block(task); + } + + #[test] + fn test_polling_cancelled_task_returns_none_with_fallible() { + let dispatcher = TestDispatcher::new(StdRng::seed_from_u64(0)); + let arc_dispatcher = Arc::new(dispatcher.clone()); + let background_executor = BackgroundExecutor::new(arc_dispatcher.clone()); + let foreground_executor = ForegroundExecutor::new(arc_dispatcher); + + let platform = TestPlatform::new(background_executor.clone(), foreground_executor.clone()); + let asset_source = Arc::new(()); + let http_client = http_client::FakeHttpClient::with_404_response(); + + let app = App::new_app(platform, asset_source, http_client); + let app_weak = Rc::downgrade(&app); + + let task = foreground_executor + .spawn_with_app(Weak::clone(&app_weak), async move { 42 }) + .fallible(); + + drop(app); + + assert!(app_weak.upgrade().is_none(), "App should have been dropped"); + + dispatcher.run_until_parked(); + + let result = background_executor.block(task); + assert_eq!(result, None, "Cancelled task should return None"); + } +}