Fix reticulate focus

[dfalbel]

When there's already a comm open for reticulate, we need to send an event message to the front-end:

ark/crates/ark/src/reticulate.rs

Lines 105 to 118 in 1eb4a8d

	if let Some(id) = comm_id_guard.deref() {
	// There's a comm_id registered, we just send the focus event
	main.get_comm_manager_tx().send(CommManagerEvent::Message(
	id.clone(),
	CommMsg::Data(json!({
	"method": "focus",
	"params": {
	"input": input_code
	}
	})),
	))?;
	return Ok(R_NilValue);
	}

I don't know of a direct way of doing this. But here, we send a message to the comm, and then forward it to the front-end
through the outgoing_tx.

This will help addressing posit-dev/positron#3865, as in that case, the quarto extension will call real_python(input=<chunk_code>) and we'll forward that code to the front end so that we can execute within the reticulate Python session.

[dfalbel]

This is not the focus of this PR, but would be nice to sneak it here because reticulate by default will send a NULL.

[lionel-]

Add a comment about this reticulate behaviour?

[lionel-]

Since these messages are supposed to be coming from the frontend, it seems a bit roundabout to forward them back to the frontend.

How about creating a global singleton representing the ReticulateService that you could dereference from the C callbacks? Maybe add a send_event() method on that service. And then you can send the event in a more straightforward manner.

What do you think?

In any case we should probably still add handling on these CommMsg from the frontend to at least log them when they come in.

[lionel-]

Add a comment about this reticulate behaviour?

[dfalbel]

The message is now sent directly to the from-end with 4695ccf and 52895fc

It might still not be ideal, because we have to clone the service in order to send it to the execution thread. We could avoid cloning by introducing a different crossbean channel like we do in the UI channel:

ark/crates/ark/src/ui/ui.rs

Line 48 in 52895fc

let (ui_comm_tx, ui_comm_rx) = crossbeam::channel::unbounded::<UiCommMessage>();

But this seemed an overkill, instead, we cleanup the global when the thread is closed:

ark/crates/ark/src/reticulate.rs

Lines 83 to 87 in 52895fc

	// Reset the global service before closing
	let mut comm_guard = RETICULATE_SERVICE.lock().unwrap();
	log::info!("Reticulate Thread closing {:?}", self.comm.comm_id);
	*comm_guard = None;

[lionel-]

oops, in hindsight it does not really make sense to make the whole service a singleton, since the service is performed from another thread. Having that global variable being a clone that could potentially run the service from another thread does not feel right.

How about storing the necessary channels in that global such as comm.outgoing_tx?

Since this will only be accessed from the R thread, it does not need to be in a Mutex. Instead you can store it in a thread_local! whose lazy intialiser panics in other threads. See

ark/crates/ark/src/plots/graphics_device.rs

Lines 65 to 68 in 7c2b7f6

	thread_local! {
	// Safety: Set once by `RMain` on initialization
	static DEVICE_CONTEXT: RefCell<DeviceContext> = panic!("Must access `DEVICE_CONTEXT` from the R thread");
	}

for an example. It'll have to be stored in a RefCell, which would cause a panic if double borrowed mutably (need to be careful), but that's better than a deadlock if double locked from the same thread.

[lionel-]

It seems like this clone is unnecessary sinceserv was moved here?

[lionel-]

This is a more conventional way of moving a clone:

        spawn!(format!("ark-reticulate-{}", comm_id), {
            let service = service.clone();
            move || {
                service
                    .handle_messages()
                    .or_log_error("Reticulate: Error handling messages");
            }
        });

[dfalbel]

I had an intermediate approach where I stored to the comm socket (and thus the necessary channels) in the global: 4695ccf

Does this sound right to you? Are there problems in storing a clone of the comm socket and calling it from the R thread (potentially at the same time as the service thread)? It seemed fine to me, but I don't know about the socket internals.

[dfalbel]

I have updated the PR to keep a gloabl reference to the socket. I'm not sure it's only accessed only by the R thread, because we need to clean it up when the current reticulate execution thread isclosing. So I think, we'll still need a mutex.

[lionel-]

I'd have made that as minimal as possible so that only the service is able to handle messages but I guess we can also just be careful.

[dfalbel]

Ok, I can probably just keep the channel the outbound.tx channel here.

[dfalbel]

@lionel- I have updated to keep just the outgoing_tx channel in global: 3e31307