Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gil: add unsafe variation for obtaining GILGuard without checks #1769

Merged
merged 1 commit into from
Aug 9, 2021

Conversation

indygreg
Copy link
Contributor

@indygreg indygreg commented Aug 8, 2021

GILGuard::acquire() cannot be called during multi-phase Python
interpreter initialization because it calls Py_IsInitialized(),
which doesn't report the interpreter as initialized until all
phases of initialization have completed.

PyOxidizer uses the multi-phase initialization API and needs to
interact with pyo3's high-level APIs (not the FFI bindings) after
partial interpreter initialization, before the interpreter is fully
initialized. Attempts to use GILGuard::acquire() result in a panic
due to the aforementioned Py_IsInitialized() check failing.

This commit introduces an unsafe function to acquire the GILGuard
without performing state checking first. I've tested this with
PyOxidizer and can confirm it allows PyOxidizer to use pyo3 APIs
before interpreter initialization is complete.

@indygreg indygreg force-pushed the force-acquire-gil branch 2 times, most recently from 8b6e8a8 to c76f78b Compare August 8, 2021 20:05
Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Thanks, this looks really interesting!

Can you elaborate on your use case for this? I'm assuming you want to mess with Python settings and import machinery before finishing main interpreter init?

One concern I have is that we might want to move away from GILGuard in the future (see also #1683). Would it be possible to expose something like this in a manner similar to Python::with_gil?

src/python.rs Outdated
Comment on lines 192 to 204
/// Acquires the [GILGuard] without performing state checking.
///
/// # Safety
///
/// This bypasses checking that [Python::acquire_gil] would normally perform, such
/// as ensuring the Python interpreter is fully initialized. If you call this
/// from a process where the Python interpreter isn't in a "good" state, the
/// process may crash.
///
/// One special case where calling this function over [Python::acquire_gil] is
/// justified is during multi-phase interpreter initialization. If the interpreter
/// is configured for multi-phase initialization, it is safe to call this function
/// between `Py_InitializeFromConfig()` and `_Py_InitializeMain()`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Acquires the [GILGuard] without performing state checking.
///
/// # Safety
///
/// This bypasses checking that [Python::acquire_gil] would normally perform, such
/// as ensuring the Python interpreter is fully initialized. If you call this
/// from a process where the Python interpreter isn't in a "good" state, the
/// process may crash.
///
/// One special case where calling this function over [Python::acquire_gil] is
/// justified is during multi-phase interpreter initialization. If the interpreter
/// is configured for multi-phase initialization, it is safe to call this function
/// between `Py_InitializeFromConfig()` and `_Py_InitializeMain()`.
/// Acquires the [`GILGuard`] without checking whether the Python interpreter
/// is fully initialized.
///
/// This bypasses checking that [`Python::acquire_gil`] would normally perform, such
/// as ensuring the Python interpreter is fully initialized.
///
/// # Safety
///
/// If you call this from a process where the Python interpreter isn't in a "good"
/// state, the process may crash.
///
/// One special case where calling this function over [`Python::acquire_gil`] is
/// justified is during multi-phase interpreter initialization. If the interpreter
/// is configured for multi-phase initialization, it is safe to call this function
/// between `Py_InitializeFromConfig()` and `_Py_InitializeMain()`.

From reading this I have a few questions:
- what is a "good" state? Is any time after calling Py_InitializeFromConfig good?
- is this function only safe to use if and only if the initialization is between Py_InitializeFromConfig() and _Py_InitializeMain()? What about afterwards?
- what is a good concrete use case for this?
- which of pyo3's api's are/aren't safe to call during this time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't give a full accounting of when this API is and is not safe to use because I'm unsure and the answer likely depends on Python interpreter implementation details.

I do know it seems to be safe between Py_InitializeFromConfig and _Py_InitializeMain.

Not all Python interpreter features are available before _Py_InitializeMain. However, many are. And so far all PyO3 functionality that PyOxidizer uses before calling _Py_InitializeMain seems to work. (But I haven't ported everything.) For an idea of what doesn't work before _Py_InitializeMain is called, you'll have to look at the CPython source code. This code path has gotten refactored heavily in Python 3.8-3.10 and TBH I'm unsure of the current state of it, so I don't want to make any false claims. The last time I looked at things, initialization stopped just before importing any .py modules on the filesystem. So you have builtin extension modules along with other core features from the interpreter. But nothing from .py modules since none are loaded yet.

src/python.rs Outdated
/// is configured for multi-phase initialization, it is safe to call this function
/// between `Py_InitializeFromConfig()` and `_Py_InitializeMain()`.
#[inline]
pub unsafe fn acquire_gil_no_checks() -> GILGuard {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub unsafe fn acquire_gil_no_checks() -> GILGuard {
pub unsafe fn acquire_gil_unchecked() -> GILGuard {

Most Rust functions like these conform to *_unchecked, which imo is clearer than *_no_checks.

Copy link
Member

Choose a reason for hiding this comment

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

👍 for calling this as-is acquire_gil_unchecked

One concern I have is that we might want to move away from GILGuard in the future (see also #1683). Would it be possible to expose something like this in a manner similar to Python::with_gil?

I think this is an excellent point; I was imagining that GilGuard::acquire will eventually itself become unsafe. At that point we could also move the initialization check from GilGuard::acquire into Python::with_gil.

So if I were to vote for the future API, I'd have:

  • Python::with_gil (as-is)
  • unsafe Python::with_gil_unchecked
  • deprecate Python::acquire_gil
  • unsafe GilGuard::acquire (which would be what this PR currently calls acquire_gil_no_checks).

Copy link
Member

Choose a reason for hiding this comment

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

(I'm thinking perhaps we can go ahead and make these changes now, for 0.15?)

GILGuard::acquire() cannot be called during multi-phase Python
interpreter initialization because it calls Py_IsInitialized(),
which doesn't report the interpreter as initialized until all
phases of initialization have completed.

PyOxidizer uses the multi-phase initialization API and needs to
interact with pyo3's high-level APIs (not the FFI bindings) after
partial interpreter initialization, before the interpreter is fully
initialized. Attempts to use GILGuard::acquire() result in a panic
due to the aforementioned Py_IsInitialized() check failing.

This commit refactors the GILGuard logic into a function that
obtains the actual GILGuard and another function to perform
checks before calling the aforementioned functions.

A new unsafe `Python::with_gil_unchecked()` has been defined
to acquire the GIL via the unchecked code path so we may obtain
a `Python` during multi-phase initialization (and possibly other
scenarios).
@indygreg indygreg force-pushed the force-acquire-gil branch from c76f78b to 3a6740a Compare August 8, 2021 22:40
@indygreg
Copy link
Contributor Author

indygreg commented Aug 8, 2021

Can you elaborate on your use case for this? I'm assuming you want to mess with Python settings and import machinery before finishing main interpreter init?

Yes. There is some more discussion in #1474. The source code I'm trying to port to pyo3 lives at https://github.com/indygreg/PyOxidizer/blob/867825be6f066241cf8bb673a781af80b0f2a788/pyembed/src/interpreter.rs#L173.

One concern I have is that we might want to move away from GILGuard in the future (see also #1683). Would it be possible to expose something like this in a manner similar to Python::with_gil?

I'll refactor to define a Python::with_gil_unchecked(), as this should be sufficient for my needs.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

👍 this looks good to me now!

I'm going to go ahead and include this in 0.14.2 - I'll make the release tomorrow now as it's late and CI is taking too long for me to want to wait up any longer anyway.

@davidhewitt davidhewitt merged commit 575c448 into PyO3:main Aug 9, 2021
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.

3 participants