pyembed: BUG: SIGABRT crash in safe code when trying to suspend the GIL

[wkschwartz]

On main at 391c74f, drop the following test function into pyembed/src/test/main_python_interpreter.rs.

#[test]
fn suspend_gil() {
    let mut interp = {
        let mut config = OxidizedPythonInterpreterConfig::default();
        config.interpreter_config.parse_argv = Some(false);
        config.set_missing_path_configuration = false;
        MainPythonInterpreter::new(config).unwrap()
    };
    let py = interp.acquire_gil().unwrap();
    py.import("sys").unwrap();
    let py = {
        interp.release_gil();
        // do work here
        interp.acquire_gil().unwrap()
    };
    py.import("builtins").unwrap();
}

The nested scope starting with let py = is necessary because interp is mutably borrowed by release_gil and acquire_gil.

Then run it to see the crash (paths shortened for brevity):

$ cargo test -- suspend_gil
   Compiling pyembed v0.11.0-pre (PyOxidizer/pyembed)
    Finished test [unoptimized + debuginfo] target(s) in 3.69s
     Running PyOxidizer/target/debug/deps/pyembed-3e29558314b7321a

running 1 test
Fatal Python error: auto-releasing thread-state, but no thread-state for this thread
Python runtime state: finalizing (tstate=0x7fc3b840c8e0)

error: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `PyOxidizer/target/debug/deps/pyembed-3e29558314b7321a suspend_gil` (signal: 6, SIGABRT: process abort signal)

Proposed fix

My theory is that MainPythonInterpreter::release_gil's API is fundamentally not useful. I propose to replace release_gil and acquire_gil with the following API:

impl<'py, 'interp, 'rsrc> MainPythonInterpreter<'py, 'interp, 'rsrc> {
    /// Returns a copy of the [GIL]-possession [token] held by `self`.
    ///
    /// [`new`] created `self` with the [GIL] held. The returned [token]'s
    /// lifetime ensures the interpreter, which is finalized when `self` is
    /// [dropped], does not die before the [GIL] is released.
    ///
    /// If you want to release the [GIL], use [`allow_threads`].
    ///
    /// # Examples
    /// ```
    /// let interp = MainPythonInterpreter::new(
    ///     pyembed::OxidizedPythonInterpreterConfig::default()).unwrap();
    /// let py = interp.py();
    /// py.eval("print('Hello, world!')").unwrap();
    /// ```
    ///
    /// [`new`]: MainPythonInterpreter::new
    /// [GIL]: https://docs.python.org/3/c-api/init.html#thread-state-and-the-global-interpreter-lock
    /// [token]: cpython::Python
    /// [dropped]: MainPythonInterpreter::drop
    /// [`allow_threads`]: MainPythonInterpreter::allow_threads
    pub fn py(&self) -> Python<'_> {
        self.gil_token
    }

    /// Release the [GIL], execute `f`, and reacquire the [GIL].
    ///
    /// [`new`] created `self` with the [GIL] held, accessible via the [`py`]
    /// method. [`allow_threads`] suspends the [GIL] during execution of `f`.
    ///
    /// # Examples
    /// ```
    /// let interp = MainPythonInterpreter::new(
    ///     pyembed::OxidizedPythonInterpreterConfig::default()).unwrap();
    /// interp.allow_threads(|| {
    ///     // Perform some difficult work on non-Python objects here.
    ///     std::thread::sleep(std::time::Duration::from_millis(10));
    /// })
    /// ```
    ///
    /// [`new`]: MainPythonInterpreter::new
    /// [`py`]: MainPythonInterpreter::py
    /// [GIL]: https://docs.python.org/3/c-api/init.html#thread-state-and-the-global-interpreter-lock
    /// [`allow_threads`]: MainPythonInterpreter::allow_threads
    pub fn allow_threads<T, F>(self, f: F) -> T where
        F: Send + FnOnce() -> T,
        T: Send,
    {
        self.py().allow_threads(f)
    }

We'd rename the existing MainPythonInterpreter::py filed to MainPythonInterpreter::gil_token: Python<'py> (no Option<>) and drop the existing MainPythonInterpreter::gil field. MainPythonInterpreter::new would acquire the GIL and never release it except temporarily surrounding the call to f in the implementation of allow_threads.

UPDATE: I now realize that both cpython and pyo3 offer allow_threads, which we can just use as the implementation. I chose the slightly stricter trait bounds of pyo3's interface in view of #324 and the fact that we can always relax the bounds later without breaking compatibility.

UPDATE 2: I took a stab at doing this locally, and updated the code above with part of what I did. I got it to compile but it requires refactoring MainPythonInterpreter::new and some other stuff I don't have the time for right now, since what I really need is #343, to which I will turn next.