Set R_MAIN_THREAD_ID at the very beginning of setup again (#580)

* Set `R_MAIN_THREAD_ID` at the very beginning of setup again

* Add an integration test for `.Rprofile`s that trigger `R_MAIN_THREAD_ID` to be checked

* Panic if `DummyArkFrontendRprofile::lock()` is called >1 times per process

* Refine note

* Use `default()` to make it clear what changes between variants

crates/ark/Cargo.toml

@@ -65,6 +65,7 @@ tracing-error = "0.2.0"
 [dev-dependencies]
 insta = { version = "1.39.0" }
 stdext = { path = "../stdext", features = ["testing"]}
+tempfile = "3.13.0"
 
 [build-dependencies]
 chrono = "0.4.23"

crates/ark/src/fixtures/dummy_frontend.rs

@@ -8,7 +8,6 @@ use std::sync::OnceLock;
 use amalthea::fixtures::dummy_frontend::DummyConnection;
 use amalthea::fixtures::dummy_frontend::DummyFrontend;
 
-use crate::interface::RMain;
 use crate::interface::SessionMode;
 
 // There can be only one frontend per process. Needs to be in a mutex because
@@ -24,6 +23,14 @@ pub struct DummyArkFrontend {
     guard: MutexGuard<'static, DummyFrontend>,
 }
 
+struct DummyArkFrontendOptions {
+    interactive: bool,
+    site_r_profile: bool,
+    user_r_profile: bool,
+    r_environ: bool,
+    session_mode: SessionMode,
+}
+
 /// Wrapper around `DummyArkFrontend` that uses `SessionMode::Notebook`
 ///
 /// Only one of `DummyArkFrontend` or `DummyArkFrontendNotebook` can be used in
@@ -34,6 +41,11 @@ pub struct DummyArkFrontendNotebook {
     inner: DummyArkFrontend,
 }
 
+/// Wrapper around `DummyArkFrontend` that allows an `.Rprofile` to run
+pub struct DummyArkFrontendRprofile {
+    inner: DummyArkFrontend,
+}
+
 impl DummyArkFrontend {
     pub fn lock() -> Self {
         Self {
@@ -44,11 +56,11 @@ impl DummyArkFrontend {
     fn get_frontend() -> &'static Arc<Mutex<DummyFrontend>> {
         // These are the hard-coded defaults. Call `init()` explicitly to
         // override.
-        let session_mode = SessionMode::Console;
-        FRONTEND.get_or_init(|| Arc::new(Mutex::new(DummyArkFrontend::init(session_mode))))
+        let options = DummyArkFrontendOptions::default();
+        FRONTEND.get_or_init(|| Arc::new(Mutex::new(DummyArkFrontend::init(options))))
     }
 
-    pub(crate) fn init(session_mode: SessionMode) -> DummyFrontend {
+    fn init(options: DummyArkFrontendOptions) -> DummyFrontend {
         if FRONTEND.get().is_some() {
             panic!("Can't spawn Ark more than once");
         }
@@ -63,25 +75,37 @@ impl DummyArkFrontend {
         let connection = DummyConnection::new();
         let (connection_file, registration_file) = connection.get_connection_files();
 
+        let mut r_args = vec![];
+
+        // We aren't animals!
+        r_args.push(String::from("--no-save"));
+        r_args.push(String::from("--no-restore"));
+
+        if options.interactive {
+            r_args.push(String::from("--interactive"));
+        }
+        if !options.site_r_profile {
+            r_args.push(String::from("--no-site-file"));
+        }
+        if !options.user_r_profile {
+            r_args.push(String::from("--no-init-file"));
+        }
+        if !options.r_environ {
+            r_args.push(String::from("--no-environ"));
+        }
+
         // Start the kernel and REPL in a background thread, does not return and is never joined.
         // Must run `start_kernel()` in a background thread because it blocks until it receives
         // a `HandshakeReply`, which we send from `from_connection()` below.
         stdext::spawn!("dummy_kernel", move || {
             crate::start::start_kernel(
                 connection_file,
                 Some(registration_file),
-                vec![
-                    String::from("--interactive"),
-                    String::from("--vanilla"),
-                    String::from("--no-save"),
-                    String::from("--no-restore"),
-                ],
+                r_args,
                 None,
-                session_mode,
+                options.session_mode,
                 false,
             );
-
-            RMain::start();
         });
 
         DummyFrontend::from_connection(connection)
@@ -113,8 +137,8 @@ impl DerefMut for DummyArkFrontend {
 impl DummyArkFrontendNotebook {
     /// Lock a notebook frontend.
     ///
-    /// NOTE: Only one of `DummyArkFrontendNotebook::lock()` re
-    /// `DummyArkFrontend::lock()` should be called in a given process.
+    /// NOTE: Only one `DummyArkFrontend` variant should call `lock()` within
+    /// a given process.
     pub fn lock() -> Self {
         Self::init();
 
@@ -125,8 +149,9 @@ impl DummyArkFrontendNotebook {
 
     /// Initialize with Notebook session mode
     fn init() {
-        let session_mode = SessionMode::Notebook;
-        FRONTEND.get_or_init(|| Arc::new(Mutex::new(DummyArkFrontend::init(session_mode))));
+        let mut options = DummyArkFrontendOptions::default();
+        options.session_mode = SessionMode::Notebook;
+        FRONTEND.get_or_init(|| Arc::new(Mutex::new(DummyArkFrontend::init(options))));
     }
 }
 
@@ -144,3 +169,61 @@ impl DerefMut for DummyArkFrontendNotebook {
         DerefMut::deref_mut(&mut self.inner)
     }
 }
+
+impl DummyArkFrontendRprofile {
+    /// Lock a frontend that supports `.Rprofile`s.
+    ///
+    /// NOTE: This variant can only be called exactly once per process,
+    /// because you can only load an `.Rprofile` one time. Additionally,
+    /// only one `DummyArkFrontend` variant should call `lock()` within
+    /// a given process. Practically, this ends up meaning you can only
+    /// have 1 test block per integration test that uses a
+    /// `DummyArkFrontendRprofile`.
+    pub fn lock() -> Self {
+        Self::init();
+
+        Self {
+            inner: DummyArkFrontend::lock(),
+        }
+    }
+
+    /// Initialize with user level `.Rprofile` enabled
+    fn init() {
+        let mut options = DummyArkFrontendOptions::default();
+        options.user_r_profile = true;
+        let status = FRONTEND.set(Arc::new(Mutex::new(DummyArkFrontend::init(options))));
+
+        if status.is_err() {
+            panic!("You can only call `DummyArkFrontendRprofile::lock()` once per process.");
+        }
+
+        FRONTEND.get().unwrap();
+    }
+}
+
+// Allow method calls to be forwarded to inner type
+impl Deref for DummyArkFrontendRprofile {
+    type Target = DummyFrontend;
+
+    fn deref(&self) -> &Self::Target {
+        Deref::deref(&self.inner)
+    }
+}
+
+impl DerefMut for DummyArkFrontendRprofile {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        DerefMut::deref_mut(&mut self.inner)
+    }
+}
+
+impl Default for DummyArkFrontendOptions {
+    fn default() -> Self {
+        Self {
+            interactive: true,
+            site_r_profile: false,
+            user_r_profile: false,
+            r_environ: false,
+            session_mode: SessionMode::Console,
+        }
+    }
+}

crates/ark/src/interface.rs

@@ -287,14 +287,10 @@ pub enum ConsoleResult {
 }
 
 impl RMain {
-    /// Sets up the main R thread and initializes the `R_MAIN` singleton. Must
-    /// be called only once. This is doing as much setup as possible before
-    /// starting the R REPL. Since the REPL does not return, it might be
-    /// launched in a background thread (which we do in integration tests). The
-    /// setup can still be done in your main thread so that panics during setup
-    /// may propagate as expected. Call `RMain::start()` after this to actually
-    /// start the R REPL.
-    pub fn setup(
+    /// Sets up the main R thread, initializes the `R_MAIN` singleton,
+    /// and starts R. Does not return!
+    /// SAFETY: Must be called only once. Enforced with a panic.
+    pub fn start(
         r_args: Vec<String>,
         startup_file: Option<String>,
         kernel_mutex: Arc<Mutex<Kernel>>,
@@ -307,6 +303,16 @@ impl RMain {
         dap: Arc<Mutex<Dap>>,
         session_mode: SessionMode,
     ) {
+        // Set the main thread ID.
+        // Must happen before doing anything that checks `RMain::on_main_thread()`,
+        // like running an `r_task()` (posit-dev/positron#4973).
+        unsafe {
+            R_MAIN_THREAD_ID = match R_MAIN_THREAD_ID {
+                None => Some(std::thread::current().id()),
+                Some(id) => panic!("`start()` must be called exactly 1 time. It has already been called from thread {id:?}."),
+            };
+        }
+
         // Channels to send/receive tasks from auxiliary threads via `RTask`s
         let (tasks_interrupt_tx, tasks_interrupt_rx) = unbounded::<RTask>();
         let (tasks_idle_tx, tasks_idle_rx) = unbounded::<RTask>();
@@ -438,13 +444,8 @@ impl RMain {
         if !ignore_user_r_profile {
             startup::source_user_r_profile();
         }
-    }
 
-    /// Start the REPL. Does not return!
-    pub fn start() {
-        // Set the main thread ID. We do it here so that `setup()` is allowed to
-        // be called in another thread.
-        unsafe { R_MAIN_THREAD_ID = Some(std::thread::current().id()) };
+        // Start the REPL. Does not return!
         crate::sys::interface::run_r();
     }
 

crates/ark/src/main.rs

@@ -12,7 +12,6 @@ use std::env;
 
 use amalthea::kernel;
 use amalthea::kernel_spec::KernelSpec;
-use ark::interface::RMain;
 use ark::interface::SessionMode;
 use ark::logger;
 use ark::signals::initialize_signal_block;
@@ -302,7 +301,8 @@ fn main() -> anyhow::Result<()> {
     // Parse the connection file
     let (connection_file, registration_file) = kernel::read_connection(connection_file.as_str());
 
-    // Set up R and start the Jupyter kernel
+    // Connect the Jupyter kernel and start R.
+    // Does not return!
     start_kernel(
         connection_file,
         registration_file,
@@ -312,9 +312,6 @@ fn main() -> anyhow::Result<()> {
         capture_streams,
     );
 
-    // Start the REPL, does not return
-    RMain::start();
-
     // Just to please Rust
     Ok(())
 }