From df91d8c42b4bbdbfa6b283e2b9fcdb33d8e13291 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Fri, 4 Oct 2024 15:11:10 -0400 Subject: [PATCH 1/2] Factor out `handle_active_request()` to correct multiline input request issue The ordering is now: - Handle input requests - Then pending lines - Then close out active requests --- crates/ark/src/interface.rs | 172 ++++++++++++++++------------ crates/ark/tests/kernel-notebook.rs | 76 ++++++++++++ crates/ark/tests/kernel.rs | 89 ++++++++++++++ 3 files changed, 266 insertions(+), 71 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 553e01481..02cee7ef3 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -640,13 +640,34 @@ impl RMain { buflen: c_int, _hist: c_int, ) -> ConsoleResult { + let info = Self::prompt_info(prompt); + log::trace!("R prompt: {}", info.input_prompt); + + // Upon entering read-console, finalize any debug call text that we were capturing. + // At this point, the user can either advance the debugger, causing us to capture + // a new expression, or execute arbitrary code, where we will reuse a finalized + // debug call text to maintain the debug state. + self.dap.finalize_call_text(); + + // TODO: Can we remove this below code? + // If the prompt begins with "Save workspace", respond with (n) + // + // NOTE: Should be able to overwrite the `Cleanup` frontend method. + // This would also help with detecting normal exits versus crashes. + if info.input_prompt.starts_with("Save workspace") { + match Self::on_console_input(buf, buflen, String::from("n")) { + Ok(()) => return ConsoleResult::NewInput, + Err(err) => return ConsoleResult::Error(err), + } + } + // We get called here everytime R needs more input. This handler // represents the driving event of a small state machine that manages - // communication between R and the frontend: + // communication between R and the frontend. In the following order: // - // - If the vector of pending lines is empty, and if the prompt is for - // new R code, we close the active ExecuteRequest and send a response to - // the frontend. + // - If we detect an input request prompt, then we forward the request + // on to the frontend and then fall through to the event loop to wait + // on the input reply. // // - If the vector of pending lines is not empty, R might be waiting for // us to complete an incomplete expression, or we might just have @@ -654,6 +675,11 @@ impl RMain { // like `foo\nbar` where `foo` is intermediate and `bar` is final). // Send the next line to R. // + // - If the vector of pending lines is empty, and if the prompt is for + // new R code, we close the active ExecuteRequest and send an + // ExecuteReply to the frontend. We then fall through to the event + // loop to wait for more input. + // // This state machine depends on being able to reliably distinguish // between readline prompts (from `readline()`, `scan()`, or `menu()`), // and actual R code prompts (either top-level or from a nested debug @@ -671,74 +697,9 @@ impl RMain { // prompt, this is a panic. We check ahead of time for complete // expressions before breaking up an ExecuteRequest in multiple lines, // so this should not happen. - - if let Some(console_result) = self.handle_pending_line(buf, buflen) { + if let Some(console_result) = self.handle_active_request(&info, buf, buflen) { return console_result; - } - - let info = Self::prompt_info(prompt); - log::trace!("R prompt: {}", info.input_prompt); - - // An incomplete prompt when we no longer have any inputs to send should - // never happen because we check for incomplete inputs ahead of time and - // respond to the frontend with an error. - if info.incomplete && self.pending_lines.is_empty() { - unreachable!("Incomplete input in `ReadConsole` handler"); - } - - // Upon entering read-console, finalize any debug call text that we were capturing. - // At this point, the user can either advance the debugger, causing us to capture - // a new expression, or execute arbitrary code, where we will reuse a finalized - // debug call text to maintain the debug state. - self.dap.finalize_call_text(); - - // TODO: Can we remove this below code? - // If the prompt begins with "Save workspace", respond with (n) - // - // NOTE: Should be able to overwrite the `Cleanup` frontend method. - // This would also help with detecting normal exits versus crashes. - if info.input_prompt.starts_with("Save workspace") { - match Self::on_console_input(buf, buflen, String::from("n")) { - Ok(()) => return ConsoleResult::NewInput, - Err(err) => return ConsoleResult::Error(err), - } - } - - if info.input_request { - if let Some(req) = &self.active_request { - // Send request to frontend. We'll wait for an `input_reply` - // from the frontend in the event loop below. The active request - // remains active. - self.request_input(req.orig.clone(), info.input_prompt.to_string()); - } else { - // Invalid input request, propagate error to R - return self.handle_invalid_input_request(buf, buflen); - } - } else if let Some(req) = std::mem::take(&mut self.active_request) { - // We got a prompt request marking the end of the previous - // execution. We took and cleared the active request as we're about - // to complete it and send a reply to unblock the active Shell - // request. - - // FIXME: Race condition between the comm and shell socket threads. - // - // Send info for the next prompt to frontend. This handles - // custom prompts set by users, e.g. `options(prompt = , - // continue = )`, as well as debugging prompts, e.g. after a - // call to `browser()`. - let event = UiFrontendEvent::PromptState(PromptStateParams { - input_prompt: info.input_prompt.clone(), - continuation_prompt: info.continuation_prompt.clone(), - }); - { - let kernel = self.kernel.lock().unwrap(); - kernel.send_ui_event(event); - } - - // Let frontend know the last request is complete. This turns us - // back to Idle. - self.reply_execute_request(req, &info); - } + }; // In the future we'll also send browser information, see // https://github.com/posit-dev/positron/issues/3001. Currently this is @@ -880,6 +841,75 @@ impl RMain { }; } + /// Returns: + /// - `None` if we should fall through to the event loop to wait for more user input + /// - `Some(ConsoleResult)` if we should immediately exit `read_console()` + fn handle_active_request( + &mut self, + info: &PromptInfo, + buf: *mut c_uchar, + buflen: c_int, + ) -> Option { + // First check if we are inside request for user input, like a `readline()` or `menu()`. + // It's entirely possible that we still have more pending lines, but an intermediate line + // put us into an `input_request` state. We must respond to that request before processing + // the rest of the pending lines. + if info.input_request { + if let Some(req) = &self.active_request { + // Send request to frontend. We'll wait for an `input_reply` + // from the frontend in the event loop in `read_console()`. + // The active request remains active. + self.request_input(req.orig.clone(), info.input_prompt.to_string()); + return None; + } else { + // Invalid input request, propagate error to R + return Some(self.handle_invalid_input_request(buf, buflen)); + } + } + + // An incomplete prompt when we no longer have any inputs to send should + // never happen because we check for incomplete inputs ahead of time and + // respond to the frontend with an error. + if info.incomplete && self.pending_lines.is_empty() { + unreachable!("Incomplete input in `ReadConsole` handler"); + } + + // Next check if we have any pending lines. If we do, we are in the middle of + // evaluating a multi line selection, so immediately write the next line into R's buffer. + // The active request remains active. + if let Some(console_result) = self.handle_pending_line(buf, buflen) { + return Some(console_result); + } + + // Finally, check if we have an active request from a previous `read_console()` + // iteration. If so, we `take()` and clear the `active_request` as we're about + // to complete it and send a reply to unblock the active Shell + // request. + if let Some(req) = std::mem::take(&mut self.active_request) { + // FIXME: Race condition between the comm and shell socket threads. + // + // Send info for the next prompt to frontend. This handles + // custom prompts set by users, e.g. `options(prompt = , + // continue = )`, as well as debugging prompts, e.g. after a + // call to `browser()`. + let event = UiFrontendEvent::PromptState(PromptStateParams { + input_prompt: info.input_prompt.clone(), + continuation_prompt: info.continuation_prompt.clone(), + }); + { + let kernel = self.kernel.lock().unwrap(); + kernel.send_ui_event(event); + } + + // Let frontend know the last request is complete. This turns us + // back to Idle. + self.reply_execute_request(req, &info); + } + + // Prepare for the next user input + None + } + fn handle_execute_request( &mut self, req: RRequest, diff --git a/crates/ark/tests/kernel-notebook.rs b/crates/ark/tests/kernel-notebook.rs index 60603d99c..9051111f2 100644 --- a/crates/ark/tests/kernel-notebook.rs +++ b/crates/ark/tests/kernel-notebook.rs @@ -104,3 +104,79 @@ fn test_notebook_execute_request_incomplete_multiple_lines() { input.execution_count ) } + +#[test] +fn test_notebook_stdin_basic_prompt() { + let frontend = DummyArkFrontendNotebook::lock(); + + let options = ExecuteRequestOptions { allow_stdin: true }; + + let code = "readline('prompt>')"; + frontend.send_execute_request(code, options); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + let prompt = frontend.recv_stdin_input_request(); + assert_eq!(prompt, String::from("prompt>")); + + frontend.send_stdin_input_reply(String::from("hi")); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] \"hi\""); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + +#[test] +fn test_notebook_stdin_followed_by_an_expression_on_the_same_line() { + let frontend = DummyArkFrontendNotebook::lock(); + + let options = ExecuteRequestOptions { allow_stdin: true }; + + let code = "val <- readline('prompt>'); paste0(val,'-there')"; + frontend.send_execute_request(code, options); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + let prompt = frontend.recv_stdin_input_request(); + assert_eq!(prompt, String::from("prompt>")); + + frontend.send_stdin_input_reply(String::from("hi")); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] \"hi-there\""); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + +#[test] +fn test_notebook_stdin_followed_by_an_expression_on_the_next_line() { + let frontend = DummyArkFrontendNotebook::lock(); + + let options = ExecuteRequestOptions { allow_stdin: true }; + + // Note, `1` is an intermediate output and is not emitted in notebooks + let code = "1\nval <- readline('prompt>')\npaste0(val,'-there')"; + frontend.send_execute_request(code, options); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + let prompt = frontend.recv_stdin_input_request(); + assert_eq!(prompt, String::from("prompt>")); + + frontend.send_stdin_input_reply(String::from("hi")); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] \"hi-there\""); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index acef3899c..acae3001a 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -415,6 +415,58 @@ fn test_stdin_basic_prompt() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); } +#[test] +fn test_stdin_followed_by_an_expression_on_the_same_line() { + let frontend = DummyArkFrontend::lock(); + + let options = ExecuteRequestOptions { allow_stdin: true }; + + let code = "val <- readline('prompt>'); paste0(val,'-there')"; + frontend.send_execute_request(code, options); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + let prompt = frontend.recv_stdin_input_request(); + assert_eq!(prompt, String::from("prompt>")); + + frontend.send_stdin_input_reply(String::from("hi")); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] \"hi-there\""); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + +#[test] +fn test_stdin_followed_by_an_expression_on_the_next_line() { + let frontend = DummyArkFrontend::lock(); + + let options = ExecuteRequestOptions { allow_stdin: true }; + + let code = "1\nval <- readline('prompt>')\npaste0(val,'-there')"; + frontend.send_execute_request(code, options); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_stream_stdout("[1] 1\n"); + + let prompt = frontend.recv_stdin_input_request(); + assert_eq!(prompt, String::from("prompt>")); + + frontend.send_stdin_input_reply(String::from("hi")); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] \"hi-there\""); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + #[test] fn test_stdin_single_line_buffer_overflow() { let frontend = DummyArkFrontend::lock(); @@ -448,3 +500,40 @@ fn test_stdin_single_line_buffer_overflow() { input.execution_count ); } + +#[test] +fn test_stdin_from_menu() { + let frontend = DummyArkFrontend::lock(); + + let options = ExecuteRequestOptions { allow_stdin: true }; + + let code = "menu(c('a', 'b'))\n3"; + frontend.send_execute_request(code, options); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + // R emits this before asking for your selection + frontend.recv_iopub_stream_stdout( + " +1: a +2: b + +", + ); + + let prompt = frontend.recv_stdin_input_request(); + assert_eq!(prompt, String::from("Selection: ")); + + frontend.send_stdin_input_reply(String::from("b")); + + // Position of selection is returned + frontend.recv_iopub_stream_stdout("[1] 2\n"); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] 3"); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} From 25fc2b167dde8b65880f16ccab23b2cdfe45d2b9 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Fri, 4 Oct 2024 15:22:15 -0400 Subject: [PATCH 2/2] Push the `Save workspace` hack into `handle_active_request()` too --- crates/ark/src/interface.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 02cee7ef3..36dd135df 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -649,18 +649,6 @@ impl RMain { // debug call text to maintain the debug state. self.dap.finalize_call_text(); - // TODO: Can we remove this below code? - // If the prompt begins with "Save workspace", respond with (n) - // - // NOTE: Should be able to overwrite the `Cleanup` frontend method. - // This would also help with detecting normal exits versus crashes. - if info.input_prompt.starts_with("Save workspace") { - match Self::on_console_input(buf, buflen, String::from("n")) { - Ok(()) => return ConsoleResult::NewInput, - Err(err) => return ConsoleResult::Error(err), - } - } - // We get called here everytime R needs more input. This handler // represents the driving event of a small state machine that manages // communication between R and the frontend. In the following order: @@ -850,6 +838,19 @@ impl RMain { buf: *mut c_uchar, buflen: c_int, ) -> Option { + // TODO: Can we remove this below code? + // If the prompt begins with "Save workspace", respond with (n) + // and allow R to immediately exit. + // + // NOTE: Should be able to overwrite the `Cleanup` frontend method. + // This would also help with detecting normal exits versus crashes. + if info.input_prompt.starts_with("Save workspace") { + match Self::on_console_input(buf, buflen, String::from("n")) { + Ok(()) => return Some(ConsoleResult::NewInput), + Err(err) => return Some(ConsoleResult::Error(err)), + } + } + // First check if we are inside request for user input, like a `readline()` or `menu()`. // It's entirely possible that we still have more pending lines, but an intermediate line // put us into an `input_request` state. We must respond to that request before processing