Run the Help comm on the R thread#1284
Conversation
| frontend.recv_iopub_busy(); | ||
| frontend.recv_iopub_execute_input(); | ||
|
|
||
| let msg = frontend.recv_iopub_comm_msg(); | ||
| assert_eq!(msg.comm_id, comm_id); | ||
| assert_eq!( | ||
| msg.data.get("method").and_then(|v| v.as_str()), | ||
| Some("show_help") | ||
| ); | ||
| assert_eq!(msg.data["params"]["kind"], "url"); | ||
| let content = msg.data["params"]["content"].as_str().unwrap(); | ||
| assert!(content.starts_with("http://127.0.0.1:")); | ||
| assert!(content.contains("plot")); | ||
|
|
||
| frontend.recv_iopub_idle(); |
There was a problem hiding this comment.
yay for busy/idle ordering stability!
|
|
||
| /// Comm ID of the UI comm, if connected. The comm itself lives in `comms` | ||
| /// like any other. | ||
| /// Comm IDs of pinned comms, if connected. |
There was a problem hiding this comment.
is "pinned" the right word? they just seem kind of "special" to me
There was a problem hiding this comment.
The idea of "pin" here is that you can't have more than one, so if a new one comes in you replace it in the slot. Agreed it's not the best term. I replaced "pinned" by "singleton".
| pub(crate) fn send_help_event(&self, event: HelpEvent) -> anyhow::Result<()> { | ||
| let Some(ref tx) = self.help_event_tx else { | ||
| return Err(anyhow!("No help channel available to handle help event. Is the help comm open? Event {event:?}.")); | ||
| let (Some(r_port), Some(proxy_port)) = (self.help_port, self.help_proxy_port) else { |
There was a problem hiding this comment.
Maybe we should store them on Console as an all or nothing pair of help_ports: Option<(u16, u16)>?
There was a problem hiding this comment.
Kind of like storing an all or nothing help_state: Option<HelpState> but without the struct
There was a problem hiding this comment.
Now Option<HelpPorts>
| let this = &*self; | ||
| handle_rpc_request(&ctx.outgoing_tx, HELP_COMM_NAME, msg, |req| { | ||
| this.handle_rpc(req) |
There was a problem hiding this comment.
ah, gotta downcast &mut self to &self to be able to pass it in the closure?
There was a problem hiding this comment.
hmm nope I think this is a leftover from a previous iteration
lionel-
force-pushed
the
task/comm-ownership
branch
from
7fb1d17 to
e687df5
Compare
lionel-
force-pushed
the
task/sync-help-comm
branch
2 times, most recently
from
b6b06b6 to
660d83b
Compare
lionel-
force-pushed
the
task/sync-help-comm
branch
from
660d83b to
d90d95f
Compare
2 participants
Branched from #1283
Progress towards #689 and #691
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).
The help comm no longer has its own thread and event loop. Instead it implements
CommHandlerand its handlers get run on the R thread.Like the UI comm, it's a pinned comm that is accessible via methods on Console, so it uses the patterns established in Simplify ownership of the list of comms #1283.
browseURL()emits a Help event to the frontend. Now that the message is emitted from the R thread, instead of the old comm thread, the message is guaranteed to be emitted within the busy/idle window of the execute-request of thebrowseURL()call. This sort of simplification of message ordering is exactly what enables accurate and stable protocol-level tests.New kernel-level integration test for the Help comm (progress towards Migrate existing comm tests to protocol-level tests #1074).