indygreg / indygreg/PyOxidizer

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

Open
#352 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
6.2k
Forks
256
PR merge metrics
No merged PRs in 30d

Description

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

```rust
#[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):
```console
$ 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:
```rust
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(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](http://dgrunwald.github.io/rust-cpython/doc/cpython/struct.Python.html#method.allow_threads) and [pyo3](https://docs.rs/pyo3/0.13.0/pyo3/struct.Python.html#method.allow_threads) 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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in pyembed/src/test/main_python_interpreter.rs and run cargo test -- suspend_gil to reproduce the SIGABRT. Read MainPythonInterpreter::release_gil, acquire_gil, py, gil, and new, then trace the proposed py and allow_threads API through their callers. Done means the suspension path no longer aborts and the affected tests and API usage are updated.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, rust
Domain
tooling
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.