ark: readline() at startup causes unstability

[lionel-]

Reported by @ryjohnson09 at https://positpbc.slack.com/archives/C05M2EZCPGR/p1705066238618889

To reproduce, insert readline("foo> ") in your Rprofile. A question is asked on startup and, if answered, causes a crash.

This also prevents renv-based workflows from working because:

• renv overrides the user's Rprofile with a project-wide Rprofile where it loads itself, e.g. https://github.com/ryjohnson09/posit-user-training/blob/main/.Rprofile
• It might ask questions to the user if it needs input, e.g. https://github.com/rstudio/renv/blob/7cdc28ba2593f3bf5651446cef7a1a85597f5145/R/load.R#L832

[lionel-]

The problem is that StdIn is a ROUTER 0MQ socket. On the Ark side, that socket normally sends responses and needs an originator ID to be able to do so. Unfortunately we don't have one until the frontend first makes a request.

As designed in the Jupyter protocol, an input_request should be nested within an execute_request sent from a given frontend (note that in theory ark might be connected to multiple frontends). However at startup there is no active execute request, and thus no originator ID for us to send a message via StdIn.

Positron does send a kernel_info_request on Shell from which we could extract an originator ID when it comes in. I'm not sure it'd work because the request will probably already be replied to at that point. If that does end up working, I worry that's it's too hacky a solution that might cause trouble with other Jupyter frontends. Also other frontends might not even send a request for kernel info (but we could simply error out on the backend side in that case). I don't have better ideas at the moment though.

Edit: I'll also try sending a dummy execute-request from positron-r right away at startup.

[DavisVaughan]

To be a little fair to ark, the experience in RStudio is weird at best. You don't see an updated prompt, so you can't see what the choices are either! But you are in a "selection" readline state!

My minimum reprex for this is to create a fully clean clone of:
https://github.com/ryjohnson09/posit-user-training/

Then open it in RStudio for the very first time

[lionel-]

@kevinushey Could renv output some clickable setup command that the user would run manually instead of running the setup at startup? Then the readline would be scoped within an execute-request and this would solve the problem at hand.

@DavisVaughan found out that running readline() in Rprofile is documented as UB.

[kevinushey]

renv tries to work around this in RStudio by running renv::load() in a separate hook, which is executed sometime "later":

https://github.com/rstudio/renv/blob/719164f10f39605fb29996f6effda35099f3006a/R/load.R#L147-L156

[DavisVaughan]

@kevinushey ah, but that never fires here! This repo (https://github.com/ryjohnson09/posit-user-training/) has an renv/ folder, so this returns "load" instead before you get there:
https://github.com/rstudio/renv/blob/719164f10f39605fb29996f6effda35099f3006a/R/load.R#L141-L145

So the load hook is not delayed, and instead continues to execute immediately, and you end up calling this ask() during the load() process (but outside the rstudio hook)
https://github.com/rstudio/renv/blob/719164f10f39605fb29996f6effda35099f3006a/R/load.R#L854

Which results in the weird screenshot I posted here:
#2070 (comment)

[jmcphers]

I think this is also what is experienced in this Slack thread:

https://positpbc.slack.com/archives/C05M2EZCPGR/p1709356630210799

I'm going to move this to RC since -- even though renv is somewhat advanced -- it seems like pretty much everyone who does use renv is going to hit it.

[jmcphers]

Another user just hit this: https://positpbc.slack.com/archives/C05M2EZCPGR/p1711471678176579

[DavisVaughan]

We think this is related to this beta user's issues once they got on an ARM version of R https://github.com/posit-dev/positron-beta/issues/179

[DavisVaughan]

I am clearing this milestone so that we can re-triage this.

My thoughts are that we should at least give a shot at trying to fix this soon-ish, even though we are unsure of exactly how it can be fixed. It essentially makes it so that anyone who uses renv will be dead in the water, which is pretty bad. And right now we don't have a good recommendation of how to even work around this (maybe we can at least avoid renv emitting a message during startup by having the user do something renv related in a terminal R session first?). I think we need some kind of plan for this for public beta.

[lionel-]

Is there any downsides to the proposal in #2070 (comment)?

Could renv output some clickable setup command that the user would run manually instead of running the setup at startup? Then the readline would be scoped within an execute-request and this would solve the problem at hand.

[DavisVaughan]

renv doesn't have any hard deps. It does suggest cli but I'm not entirely sure why. That might be a good approach though if we can convince Kevin

[juliasilge]

Hitting this again, I believe: https://positpbc.slack.com/archives/C05M2EZCPGR/p1716565623908479

[DavisVaughan]

Noting this for easy access, in ?Rprofile, under the Note section it says:

It is not intended that there be interaction with the user during startup code. Attempting to do so can crash the R process.

And readline() counts as interaction with the user

[kevinushey]

I'm happy to fix this in renv; do we know where readline() is being triggered? (I can investigate too but if someone already has that information handy that would help)

[jennybc]

Summary of what we're going to do here:

• Tweaks to renv that include reordering some checks during autoload, making sure the delayed load in the session init hook actually runs, etc etc. This will make renv and Positron play better together going forward, i.e. people will need to update renv. PR in flight: Refine auto-loading behaviour rstudio/renv#1915
• Change in ark that catches too-early attempts to run functions like readline() or menu(). The generic handler will emit an error (vs. segfaulting and failing to start R). There will likely be special handling for renv autoloading, which will implement some sort of default/non-interactive behaviour. This addresses existing renv-using projects. PR in flight: Take full control of sourcing .Rprofile and .Rprofile.site ark#383

• Future work in ark to develop an equivalent of RStudio's session init hook. Tracked in ark: Support a session init hook ark#697.

[juliasilge]

In Positron 2024.06.0 (Universal) build 50, I no longer see crashes in these situations.

With readline("foo> ") in my Rprofile:

Opening up a fresh clone of https://github.com/ryjohnson09/posit-user-training/:

My console is fully functional in both of these cases and there is no crash.