indygreg / indygreg/PyOxidizer
pyembed: BUG: SIGABRT crash in safe code when trying to suspend the GIL
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
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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