Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor prompt detection #51

Merged
merged 6 commits into from
Jun 26, 2023
Merged

Refactor prompt detection #51

merged 6 commits into from
Jun 26, 2023

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Jun 23, 2023

Progress towards posit-dev/positron#535.

To solve readline interrupts we'll need to detect readline prompt from r_read_console(). This PR creates a new PromptInfo struct that contains all information about the prompt type (user request? incomplete code?). This information is created synchronously in r_read_console() before being sent away to other threads. Previously the prompt type was detected from the ark-execution thread by querying R state without taking the R lock which is unsafe.

The PR also improves the way prompt types are detected. Currently the prompt string is compared to getOption("prompt") and getOption("continue"). This could be fooled by e.g. readline("> ") or readline("+ "). Instead, we now call sys.nframe() to detect whether we are at top-level. If that's the case, then we can compare the prompt to getOption("continue") safely.

In a future PR for posit-dev/positron#407 we'll also detect browser prompts using the equivalent of rlang::env_is_browsed(sys.frame(sys.nframe())), i.e. inspecting the last frame on the stack to see if it has been marked for debugging. If a browser prompt, we'll consider that it can't be a readline prompt. There are very rare edge cases where this might not be true, e.g. debug(readline), but I don't think we can do better without getting this information from R itself in an implove ReadConsole() callback.

Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

crates/ark/src/interface.rs Outdated Show resolved Hide resolved
fn init_interface() {
unsafe {
let nframe_call = crate::r_lang!(crate::r_symbol!("sys.nframe"));
R_PreserveObject(nframe_call);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can preserve these using our own internal mechanism (to avoid accumulating a potentially large "bag" of preserved objects)? Is that worth doing?

That is, have an internal "precious list" that we append / remove these from, which is itself preserved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main advantage of preserving in local storage instead of the global precious list is that it makes it easier to analyse the source of leaks using r-lib/memtools.

crates/harp/src/session.rs Outdated Show resolved Hide resolved
SESSION_INIT.call_once(init_interface);

unsafe {
let ffi = r_try_eval_silent(NFRAME_CALL as SEXP, R_BaseEnv)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth considering putting together tools for interacting with R_GlobalContext to learn these sorts of things about the call stack? (I think that might be necessary in the future, for certain intermediate frames that R doesn't include in the user-facing R functions?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a possibility. Probably best to avoid if we can though?

@lionel- lionel- force-pushed the bugfix/prompt-type branch from bf71e82 to 91612f3 Compare June 26, 2023 20:48
@lionel- lionel- merged commit 3916492 into main Jun 26, 2023
@lionel- lionel- deleted the bugfix/prompt-type branch June 26, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants