Simplify ownership of the list of comms#1283
Merged
Merged
Conversation
DavisVaughan
approved these changes
Jul 3, 2026
Comment on lines
+133
to
+139
| // Snapshot the `Rc`s so the `comms` borrow is dropped before we run any | ||
| // handler (a handler may reenter `self.comms`). | ||
| let comms: Vec<Rc<ConsoleComm>> = self.comms.borrow().values().map(Rc::clone).collect(); | ||
| for comm in comms { | ||
| comm.handler | ||
| .borrow_mut() | ||
| .handle_environment(event, &comm.ctx); |
Contributor
There was a problem hiding this comment.
So, to be clear, a handler may reenter self.comms but it CANNOT call comm.handler.borrow_mut() reentrantly on itself again, right? Otherwise that's a panic at debug time, and we log at release time
Would be nice if we can avoid reentrant issues entirely, but I get it
Comment on lines
156
to
180
| /// Remove a comm from the map, keeping the UI index in sync. | ||
| fn remove_comm(&self, comm_id: &str) -> Option<Rc<ConsoleComm>> { | ||
| let comm = self.comms.borrow_mut().remove(comm_id)?; | ||
|
|
||
| /// Take the UI comm only if its `comm_id` matches. Checks and takes | ||
| /// in a single `borrow_mut()` so there is no TOCTOU gap. | ||
| fn take_ui_comm_if(&self, comm_id: &str) -> Option<ConsoleComm> { | ||
| let mut guard = self.ui_comm.borrow_mut(); | ||
| if guard.as_ref().is_some_and(|ui| ui.comm_id == comm_id) { | ||
| guard.take() | ||
| } else { | ||
| None | ||
| let mut ui_comm_id = self.ui_comm_id.borrow_mut(); | ||
| if ui_comm_id.as_deref() == Some(comm_id) { | ||
| *ui_comm_id = None; | ||
| } | ||
| } | ||
|
|
||
| fn set_ui_comm(&self, ui: ConsoleComm) { | ||
| *self.ui_comm.borrow_mut() = Some(ui); | ||
| Some(comm) | ||
| } | ||
|
|
||
| // -- Comms map helpers ------------------------------------------------ | ||
|
|
||
| /// Take a comm out, call `f`, put it back. | ||
| fn with_comm_mut(&self, comm_id: &str, f: impl FnOnce(&mut ConsoleComm)) { | ||
| let Some(mut comm) = self.take_comm(comm_id) else { | ||
| log::warn!("Received message for unknown registered comm {comm_id}"); | ||
| /// Close and drop the currently registered UI comm, if any. | ||
| fn close_ui_comm(&self) { | ||
| let Some(old) = self | ||
| .ui_comm_id | ||
| .borrow_mut() | ||
| .take() | ||
| .and_then(|old_id| self.comms.borrow_mut().remove(&old_id)) | ||
| else { | ||
| return; | ||
| }; | ||
| f(&mut comm); | ||
| self.comms.borrow_mut().insert(comm.comm_id.clone(), comm); | ||
| } | ||
|
|
||
| fn take_comm(&self, comm_id: &str) -> Option<ConsoleComm> { | ||
| self.comms.borrow_mut().remove(comm_id) | ||
| log::info!("Replacing an existing UI comm."); | ||
| old.handler.borrow_mut().handle_close(&old.ctx); | ||
| } |
Contributor
There was a problem hiding this comment.
These don't feel quite right to me
In particular
log::info!("Replacing an existing UI comm.");
that doesn't really align with what happens. you actually remove it here and set it to None but don't replace it here. you replace it in the caller.
i kind of felt like we don't actually need this helper. like it feels like in comm_open_frontend we should do remove_comm(ui_comm_id) (or maybe comm_handle_close, so it does handle_close) followed up with setting the ui_comm_id to the new one.
lionel-
force-pushed
the
task/comm-ownership
branch
from
7fb1d17 to
e687df5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Progress towards #689 and #691
Follow-up to #1161 (see
console_comm.rschanges)See #1075 for context: we're moving comms out of their own threads to run on the R thread, to simplify the concurrency structure of Ark and remove the risk of concurrent R evaluations which are UB (although these are now also tackled under a different angle, see #1222).
In #1161 I made progress towards phasing out unsafe mutation on Console state (see #1145) by using interior mutability in Console methods. To manage comm state in particular, I went for a take/move approach that removes a comm from the map of comms owned by the Console, and passes the moved comm by value to its handlers. This allowed data explorer comm handlers to manipulate the comm map and insert new explorer comms (when an inline explorer gets promoted to a full explorer).
Moving the comm was awkward with the UI comm though, which needed to be pinned down because its handlers might cause Console events that themselves require the UI comm (e.g. to send a message to the frontend). For this reason the UI comm used a different dispatch strategy with a shared borrow, that required ad hoc infrastructure.
The Help comm is also accessible from Console methods and is going to need a similar setup, so I thought I'd go ahead and try to simplify this:
All comms now live in the map, including the UI comm. But they are now wrapped in an
Rc.The dispatcher now holds an
Rcclone of the comm rather than an exclusively owned value. The comm stays in the Console-owned map during dispatch, which is how they remain accessible for reentrancy while a handler is running. Thanks to the Rc wrapping and cloning, the comm map remains fully owned by the Console during dispatch and can be manipulated while a handler is running, allowing data explorers to clone/promote themselves.Note that the handler's
&mutcomes from interior mutability of the comm handler inside theConsoleComm. So the one invariant we need to guarantee is that we never dispatch a comm's handler reentrantly.The UI comm still gets some ad hoc handling because of its "pinned" nature (there can only be one UI comm at any time, so opening a new one drops the old comm if any).