Skip to content

Conversation

@lionel-
Copy link
Contributor

@lionel- lionel- commented Jun 26, 2023

Replaces #54.
Branched from #52

This refactoring intends to reduce the complexity of our concurrency model by incorporating as much functionality as possible into the R thread.

  • The ReadConsole() callback now pulls execution requests from the socket-shell thread in place of the ark-execution thread which is now removed. Other requests that need to run concurrently to R are run from a new ark-shell thread owned by Shell.

  • Correspondingly the Request enum is split into two enums RRequest (requests for the main thread) and KernelRequest (requests for the Kernel object shared among R, Shell, and LSP).

  • The execute request/response methods now live in interface.rs and no longer communicate across threads via channels. Everything in interface.rs runs synchronously with R and so this code may now be considered single-threaded regarding the use of R and conceptually does not need the R lock (though we still should use r_lock! to protect against interrupts or perhaps add another macro for that purpose). This should make it easier to reason about, esp. in terms of message-passing races e.g. with interrupts.

  • Inputs are now returned synchronously to ReadConsole() and represented by a new ConsoleInput enum with variants EOF and Input(String).

  • The R_ERROR_OCCURRED atomic and its friends are now simple variables in RMain.

  • Some methods have been moved from Kernel to RMain methods. As discussed in Collect all global state for R callbacks in a struct #52, I would like to move all frontend methods to RMain in an ulterior PR, on the model of ffb6bbf. This way the methods can be written more conventionally with state in self, and we just need to make sure the unsafe dereferences of the global R_MAIN object are indeed made from the R thread to ensure safety.

  • For simplicity, the StdIn channel is no longer passed via a specific message but is passed to Kernel.connect() by argument. This channel is now owned by RMain.

@lionel- lionel- requested review from jmcphers and kevinushey June 26, 2023 21:43
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!

@lionel- lionel- changed the base branch from refactor/read-console-state to main June 28, 2023 07:25
@lionel- lionel- force-pushed the refactor/ark-execution branch from ca8ad51 to 518dbf8 Compare June 28, 2023 07:32
@lionel- lionel- force-pushed the refactor/ark-execution branch from b1344bb to 099844d Compare June 28, 2023 07:41
@lionel- lionel- merged commit 6f15294 into main Jun 28, 2023
@lionel- lionel- deleted the refactor/ark-execution branch June 28, 2023 07:42
Comment on lines +204 to +207
/// Represents whether an error occurred during R code execution.
pub error_occurred: bool,
pub error_message: String, // `evalue` in the Jupyter protocol
pub error_traceback: Vec<String>,
Copy link
Contributor

@DavisVaughan DavisVaughan Jul 6, 2023

Choose a reason for hiding this comment

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

Might be worth aggregating into something like

pub error: Option<RError>

// with this separately as the error
struct RError {
  pub message: String,
  pub traceback: Vec<String>
}

I can think about that

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.

5 participants