From 28dc83f076169cf1242d235ea198437c2f8eea5d Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Sat, 27 Dec 2025 10:38:12 -0800 Subject: [PATCH] Add thread safety constraints --- async-app-result-removal-tracker.json | 36 ++++++++++++++++++--------- async-app-result-removal.md | 33 +++++++++++++++++------- 2 files changed, 48 insertions(+), 21 deletions(-) diff --git a/async-app-result-removal-tracker.json b/async-app-result-removal-tracker.json index 28aa8d53686d6b4d628146540e041ed1b0912953..12d4073ef6d302b79a2f1a1a1d20fd55955218c9 100644 --- a/async-app-result-removal-tracker.json +++ b/async-app-result-removal-tracker.json @@ -9,48 +9,60 @@ "tasks": [ { "id": "1.1", - "description": "Add Option> field to RunnableMeta struct", - "file": "crates/gpui/src/executor.rs", + "description": "Add MainThreadWeak newtype with unsafe Send+Sync impls to platform.rs", + "file": "crates/gpui/src/platform.rs", "status": "not_started" }, { "id": "1.2", - "description": "Update trampoline function in Mac dispatcher to check app status before run()", - "file": "crates/gpui/src/platform/mac/dispatcher.rs", + "description": "Update RunnableMeta to use Option> for app field", + "file": "crates/gpui/src/platform.rs", "status": "not_started" }, { "id": "1.3", - "description": "Update trampoline function in Test dispatcher to check app status before run()", - "file": "crates/gpui/src/platform/test/dispatcher.rs", + "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" }, { "id": "1.4", - "description": "Modify ForegroundExecutor::spawn_with_priority to accept optional app weak pointer", - "file": "crates/gpui/src/executor.rs", + "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" }, { "id": "1.5", - "description": "Update AsyncApp::spawn to pass app weak pointer to executor", - "file": "crates/gpui/src/app/async_context.rs", + "description": "Add ForegroundExecutor::spawn_with_app that accepts Weak", + "file": "crates/gpui/src/executor.rs", "status": "not_started" }, { "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" + }, + { + "id": "1.7", + "description": "Update AsyncWindowContext::spawn to pass app wrapped in MainThreadWeak", + "file": "crates/gpui/src/app/async_context.rs", + "status": "not_started" + }, + { + "id": "1.8", "description": "Write unit test verifying task cancellation when app is dropped", "file": "crates/gpui/src/executor.rs", "status": "not_started" }, { - "id": "1.7", + "id": "1.9", "description": "Write unit test for nested tasks both cancelling cleanly", "file": "crates/gpui/src/executor.rs", "status": "not_started" }, { - "id": "1.8", + "id": "1.10", "description": "Create async_cancellation.rs example demonstrating behavior", "file": "crates/gpui/examples/async_cancellation.rs", "status": "not_started" diff --git a/async-app-result-removal.md b/async-app-result-removal.md index f144ed2b1c3672826800cd6cd46544c4656ec7da..4d026e8ee7b4c8592ca150ef878d83b0f58d6a6a 100644 --- a/async-app-result-removal.md +++ b/async-app-result-removal.md @@ -130,27 +130,42 @@ The `Flatten` trait in `gpui.rs` exists solely to handle `Result>` whe ### RunnableMeta -Add optional app weak pointer: +Add optional app weak pointer using a `MainThreadWeak` newtype that unsafely implements `Send + Sync`: + ```rust +/// Weak reference that can cross thread boundaries but must only be accessed on the main thread. +/// SAFETY: Only create, access (upgrade), and drop on the main thread. +pub struct MainThreadWeak(Weak); +unsafe impl Send for MainThreadWeak {} +unsafe impl Sync for MainThreadWeak {} + pub struct RunnableMeta { pub location: &'static Location<'static>, - pub app: Option>, // NEW + pub app: Option>, // NEW } ``` +**Safety invariants:** +- `app` is `Some` only for foreground tasks (spawned via `ForegroundExecutor::spawn_with_app`) +- `ForegroundExecutor` is `!Send`, so spawn only happens on main thread +- Trampolines run on main thread, so `upgrade()` is main-thread only +- Background tasks always use `app: None` + ## Implementation Phases -### Phase 1: Add Trampoline Check Infrastructure (macOS First) +### Phase 1: Add Trampoline Check Infrastructure (macOS + Test) -Start with macOS to validate the approach before implementing on other platforms. +Start with macOS and Test dispatcher to validate the approach. -1. Modify `RunnableMeta` to include `Option>` -2. Update trampoline function in Mac dispatcher to check app status before running: +1. Add `MainThreadWeak` newtype to `crates/gpui/src/platform.rs` with unsafe `Send + Sync` impls +2. Update `RunnableMeta` to use `Option>` for app field +3. Update Mac dispatcher trampoline to check `unsafe { app.upgrade() }` before `run()`: - `crates/gpui/src/platform/mac/dispatcher.rs` -3. Update trampoline function in Test dispatcher (needed for tests): +4. Update Test dispatcher `tick()` to check `unsafe { app.upgrade() }` before `run()`: - `crates/gpui/src/platform/test/dispatcher.rs` -4. Modify spawn paths in `AsyncApp` to populate the app weak pointer in metadata -5. Write tests to validate cancellation behavior on macOS +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 ### Phase 2: Extend to Other Platforms