posit-dev / posit-dev/positron

Revisit uses of RPC `callMethod` that (inappropriately) require kernel to be idle

Open
#4,641 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

area: core
Dominant language
TypeScript
Stars
4.3k
Forks
184
Avg merge
1d 11h
Merged PRs (30d)
206

Description

While working on https://github.com/posit-dev/positron/issues/4606 on passing env vars from the R session, @lionel- pointed out that our uses of callMethod() in the R extension do not resolve when the kernel is busy. This may look broken to users who, for example, may have a Shiny app running while they are trying to run some command. As of today, the uses we have are:

  • 'get_locale' to get the R locale for package testing
  • 'is_installed' to check if an R package is installed
  • newly added 'get_env_vars' for passing env vars into a package development "task"
  • a few other than I am less sure are problematic in terms of timing ('setConsoleWidth', 'isPackageAttached', 'showHelpTopic', 'showVignetteTopic')

Here are details on possible different approaches to mitigate this:

Timing

Right now the callMethod() call will only resolve when the kernel becomes idle. A better way of getting these envvars would be to push them from ark to positron after each top-level command. Positron would save the current values of these variables and use them as needed when creating tasks. This way, tasks can be run at any time rather than just when R is idle. This is not necessarily a blocker for this PR since quite often R is idle during package development, but this is still a regression for the busy case. I also wonder about the user experience as the action will not have immediate feedback in some cases, which would be confusing. So it's probably worth it to implement the push approach now.

I see two ways to go about it:

  1. Push from the kernel to positron by sending a backend-to-frontend event.

  2. Push from the kernel to the LSP. Then add an LSP request that the frontend can use without having to wait.

(1) seems the hardest (surprisingly to me, and @jmcphers might see an easier approach than I can). First it depends on who should process these events. Should it be a language-agnostic part of positron or only positron-r? If the former, it's a matter of adding the new event type on the UI comm following existing cases. If the latter, I think the UI comm is not the best place for this and we might want to create a new positron-r-specific comm that would get these events, which I think has not been done before. The advantage of such a comm is that it could be developed and experimented with "in extension" without changing core parts of positron. Once done, it should be easy to create events targetting only the positron-r extension.

In both cases, the kernel will need to to compute the current value of the envvars and send them as some kind of event, and the place to do it is https://github.com/posit-dev/ark/blob/d1f72cb03be0a2c3add85c59c147067275659eb7/crates/ark/src/interface.rs#L659.

(2) is the easiest, it requires adding the envvars as part of the ConsoleInput that we already send to the LSP after each top-level command in refresh_lsp. Then we need to implement a new LSP request to get these values. You can follow what we did for statement range for instance, starting from statement_range. I'd be happy to help with this if needed, we could pair on this for instance.

Originally posted by @lionel- in https://github.com/posit-dev/positron/issues/4606#issuecomment-2340082864

We don't think this is likely to come up for people doing R package development, but it likely will come up for people who want to create their own tasks.

For the specific application of env vars, we might consider language agnostic infrastructure, since certainly this doesn't only apply to R. From @jmcphers:

The ability to see what environment variables are currently set in the kernel is a very useful tool in a lot of contexts, so I think it is worth considering some language-agnostic supporting infrastructure around pushing/synchronizing the set to the front end as you describe.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the R extension's callMethod() uses, then read crates/ark/src/interface.rs at the ConsoleInput and refresh_lsp paths and crates/ark/src/lsp/backend.rs at statement_range. Compare the proposed event and LSP-request approaches, including the other listed calls. Done means the affected values and calls no longer depend on the kernel becoming idle, with the chosen cross-language behavior documented and verified.

Written by the indexing model from the issue text.

Assessment

Tech stack
r, rust, typescript
Domain
backend-api-design, developer-experience, full-stack
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.