From 8c86a4b9a26b3eed3eb5e39bcb9e10f4a044194a Mon Sep 17 00:00:00 2001 From: ymote <151983+ymote@users.noreply.github.com> Date: Sat, 10 Oct 2026 16:00:46 -0700 Subject: [PATCH] services: tell services when an isolate closes Replier::isolate_key is an address: once an isolate is gone, a new one can get the same key, so a service that keeps state per isolate cannot tell a reopened app from the one that closed. OctoSense's pdf service (OctoSense #472) keeps open documents per isolate; a reopened PDF Tools could inherit its predecessor's documents, which count toward its cap. on_isolate_closed registers a listener that cancel_heap calls with the closed isolate's heap key, after the isolate's pending requests and queued answers are cleared and with no lock of this module held, on the thread closing it, before a new isolate can reuse the key. Listeners live in their own mutex, not under SERVICES (dispatch holds that lock while a service runs). Registering the same function twice registers it once. Co-Authored-By: Claude Opus 5.5 --- crates/appstore/src/services.rs | 71 +++++++++++++++++++++++++++++++-- docs/CODE-WALKTHROUGH.md | 5 ++- 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/crates/appstore/src/services.rs b/crates/appstore/src/services.rs index 7766d2e..fb1fad9 100644 --- a/crates/appstore/src/services.rs +++ b/crates/appstore/src/services.rs @@ -32,8 +32,10 @@ //! service raised a sheet waits for the person instead, and its clock starts //! again when the sheet closes. A host drops an isolate's requests and //! answers when the isolate closes ([`cancel_heap`]), so nothing reaches a -//! replacement. The requests waiting, their arguments and their answers are -//! bounded, and malformed arguments are refused rather than read as `null`. +//! replacement, and a service that keeps state per isolate hears of it +//! ([`on_isolate_closed`]) before the isolate's heap key can be reused. The +//! requests waiting, their arguments and their answers are bounded, and +//! malformed arguments are refused rather than read as `null`. //! //! **The surface decides about sheets.** Every call carries `may_prompt`, //! the isolate's surface permission: an app in the foreground may have a @@ -125,7 +127,10 @@ impl Replier { /// Native-only identity of the requesting isolate. A service may use this /// to snapshot that isolate's existing storage policy before asynchronous /// work. It is not authorization, must never come from script arguments, - /// and must not be retained as proof that an isolate is still alive. + /// and must not be retained as proof that an isolate is still alive: it + /// is an address, which a new isolate can reuse once this one is gone. A + /// service that keeps state per isolate drops it when + /// [`on_isolate_closed`] tells it the isolate closed. pub fn isolate_key(&self) -> usize { self.heap_key } @@ -240,9 +245,32 @@ fn start_sweeper() { /// Forget an isolate that is closing: its waiting requests and its queued /// answers. A late answer for it goes nowhere, and never to a replacement. +/// Then the services' [`on_isolate_closed`] listeners hear its heap key. pub fn cancel_heap(heap_key: usize) { with_pending(|pending| pending.retain(|(heap, _), _| *heap != heap_key)); REPLIES.lock().unwrap().retain(|(heap, _, _, _)| *heap != heap_key); + // Copied out, so no lock of this module is held while a listener runs. + let listeners = CLOSED_LISTENERS.lock().unwrap_or_else(|e| e.into_inner()).clone(); + for listener in listeners { + listener(heap_key); + } +} + +/// The services' listeners for closing isolates ([`on_isolate_closed`]). +/// Not under [`SERVICES`]: `dispatch` holds that lock while a service runs. +static CLOSED_LISTENERS: Mutex> = Mutex::new(Vec::new()); + +/// Call `listener` with an isolate's heap key when the isolate closes +/// ([`cancel_heap`]), on the thread closing it (the UI thread), before any +/// new isolate can reuse the key. For services that keep state per isolate: +/// [`Replier::isolate_key`] is an address, reused once the isolate is gone. +/// A listener must be quick and must not call back into this module. +/// Registering the same function again changes nothing. +pub fn on_isolate_closed(listener: fn(usize)) { + let mut listeners = CLOSED_LISTENERS.lock().unwrap_or_else(|e| e.into_inner()); + if !listeners.iter().any(|known| std::ptr::fn_addr_eq(*known, listener)) { + listeners.push(listener); + } } /// A request's arguments, or why it is refused before any service sees it. @@ -715,6 +743,43 @@ mod tests { assert!(take_replies_for(&[7104]).is_empty(), "nothing reaches a replacement isolate"); } + /// A service that keeps state per isolate hears each closing isolate's + /// heap key once, however often it registered. + #[test] + fn a_closing_isolate_reaches_each_listener_once() { + static HEARD: Mutex> = Mutex::new(Vec::new()); + fn listener(heap: usize) { + HEARD.lock().unwrap().push(heap); + } + on_isolate_closed(listener); + on_isolate_closed(listener); + cancel_heap(7110); + // Other tests close isolates of their own meanwhile: count this one. + let heard = HEARD.lock().unwrap().iter().filter(|heap| **heap == 7110).count(); + assert_eq!(heard, 1, "the key, once: a second registration is the same listener"); + } + + /// A listener runs once the isolate's requests and answers are gone, and + /// with none of this module's locks held. + #[test] + fn a_closed_isolates_listener_runs_after_its_queues_are_cleared() { + static LEFT: Mutex> = Mutex::new(None); + fn listener(heap: usize) { + if heap == 7111 { + let waiting = with_pending(|pending| pending.keys().any(|(h, _)| *h == heap)); + let answered = REPLIES.lock().unwrap().iter().any(|(h, ..)| *h == heap); + *LEFT.lock().unwrap() = Some((waiting, answered)); + } + } + let held = holds("closing_listener", None, false); + dispatch(call("closing_listener.read"), 7111, 1, &mut Host::default()); + dispatch(call("closing_listener.read"), 7111, 2, &mut Host::default()); + held.lock().unwrap().pop().unwrap().send(Ok(Value::Null)); // answered, not yet taken + on_isolate_closed(listener); + cancel_heap(7111); + assert_eq!(*LEFT.lock().unwrap(), Some((false, false)), "nothing of the isolate is left when its listeners hear of it"); + } + #[test] fn a_background_surface_cannot_raise_a_sheet() { let _held = holds("signin_bg", None, true); diff --git a/docs/CODE-WALKTHROUGH.md b/docs/CODE-WALKTHROUGH.md index d83fd48..0dcca60 100644 --- a/docs/CODE-WALKTHROUGH.md +++ b/docs/CODE-WALKTHROUGH.md @@ -257,7 +257,10 @@ Take an admitted app that calls `host.request("mail.list", args, callback)`: 5. A later `pump` delivers the result to the original isolate's callback. A late or duplicate reply is ignored. Closing an app calls `cancel_heap` for its isolate and for its host sheet's, so a reply meant for either never - reaches a replacement app instance. + reaches a replacement app instance. A service that keeps state per + isolate registers `services::on_isolate_closed`: `cancel_heap` then calls + it with the closed isolate's heap key, after clearing its queues, before a + new isolate can reuse the key (a heap key is an address). A request waits at most 60 seconds by default; a service may set its own timeout. Each isolate may have 32 requests pending, and all isolates together