Conversation
|
|
||
| // The comm_open triggers busy/idle on the shell thread and also | ||
| // sends an EstablishUiCommChannel kernel request to the console. | ||
| // The UI comm then sends events (prompt_state, working_directory) | ||
| // as CommMsg on IOPub. These can arrive in any order relative to | ||
| // the busy/idle. We wait for the prompt_state CommMsg as evidence | ||
| // that the UI comm has been established, draining everything. | ||
| let deadline = Instant::now() + RECV_TIMEOUT; | ||
| let mut got_prompt_state = false; | ||
| let mut idle_count = 0u32; | ||
| // The UI comm runs on the R thread via CommHandler. The comm_open | ||
| // blocks Shell while the handler's `handle_open()` runs, so events | ||
| // arrive deterministically within the Busy/Idle window. | ||
| self.recv_iopub_busy(); |
| // NOTE: This is a legit use of interrupt-time task. No R access here, and | ||
| // we need to go through Console since it owns the UI comm. |
There was a problem hiding this comment.
Could update this note to reflect our thoughts about the fact that this could be a crossbeam channel notification that the Console knows about, without needing an r_task
There was a problem hiding this comment.
hmm... I think I'd still see it modelled as some kind of r_interrupt_task() that receives a trimmed down Console as input.
| }; | ||
|
|
| if comm_name == UI_COMM_NAME { | ||
| if let Some(old_id) = self.ui_comm_id.take() { | ||
| log::info!("Replacing an existing UI comm."); | ||
| if let Some(mut old) = self.comms.remove(&old_id) { | ||
| old.handler.handle_close(&old.ctx); | ||
| } | ||
| } | ||
| self.ui_comm_id = Some(comm_id.clone()); | ||
| } |
There was a problem hiding this comment.
Something feels slightly off that here and in comm_handle_close you have to specially handle the ui_comm_id but I can't quite figure out how you'd simplify it
There was a problem hiding this comment.
I've extracted into comm_remove() so all special handling is centralised.
lionel-
force-pushed
the
task/sync-ui-comm
branch
from
ef3eb59 to
adc727f
Compare
lionel-
force-pushed
the
task/sync-ui-comm
branch
2 times, most recently
from
54b06d5 to
bd54059
Compare
lionel-
force-pushed
the
task/sync-ui-comm
branch
from
bd54059 to
be02717
Compare
|
Green run on the frontend side: https://github.com/posit-dev/positron/actions/runs/23299947186/job/67757907790?pr=12542 |
2 participants
Progress towards #689
UI comm now accessible via
Console::ui_comm(). It returns an optional reference to a UI comm view with methods to send UI comm events or requests. If the option isNone, the UI comm is not connected. If it'sSome().The
Console::try_ui_comm()variant returns aResultand is nice for easy propagation of errors when the UI comm is expected.Since the UI comm is frontend-initiated, added
CommOpenplumbing. You were right to call out the complicated-looking factory handler, it was not necessary (and I can't think of a reason for it to be necessary with other comms).The continuation prompt field has been removed from
PromptInfo. We now refresh the prompt from thehandle_environment()comm method, and fetchgetOption("continue")from there.Fixes the busy/idle race condition, and makes the tests both simpler, more robust, and more exhaustive.