python: deprecate Python::acquire_gil

[davidhewitt]

From discussion in #1683 and #1769

• Deprecates Python::acquire_gil.
• Makes Python::with_gil always acquire the GIL (removing EnsureGIL optimization).
• Makes GILGuard::acquire_gil an unsafe public API.
• Removes the drop order safety check from GILGuard::drop.

Not mergeable because there's a ton of deprecation warnings. Needs #1786 and maybe one other PR to migrate PyO3's own codebase from acquire_gil -> with_gil.

[mejrs]

Makes GILGuard::acquire_gil an unsafe public API.

I think that this is a mistake and that having this is too confusing.

This function is not crystal clear about how it's supposed to be used safely.

The "interpreter needs to be initialized", what does that mean? How do I do that? How do I know that it is? It's not clear from the context whether I need to call some function first or it's a config somewhere that needs to be set. And then the GILGuards need to be dropped in order - this sounds like something where it's easy to get a corner case where this goes wrong unnoticed. (also I'm guessing it would be a really bad idea to mem::forget one of these)

In contrast a function like Python::assume_gil_acquired communicates this better; if you use the token it gives you (or anything derived from it) in any context where you're not actually holding the GIL then, well, you get yourself some UB.

The name is too attractive.

When you're new and looking through the docs, this looks a lot like "this is what I need". Certainly more so than Python::with_gil. I'm concerned this nudges people into using this unsafe function everywhere and this is undesirable, especially when it's something that "seems to work".

Its purpose.

Where is this function useful? Can it do things Python::with_gil can't? If yes, then we have to accept that people are sometimes forced to use this function, in which case it being unsafe is undesirable. If no, then why do we have it?

[mejrs]

This needs doc examples showing correct and incorrect usage.

[davidhewitt]

Thanks, that's really great points.

I think what I'll do is remove GILGuard::acquire_gil from this PR. It's always easy to add it as a new API later if users turn up with cases that Python::with_gil doesn't solve.

[mejrs]

👍

Does that mean GILGuard will be deprecated/private entirely?

[davidhewitt]

Yes, I guess so. I think that's a good thing. (Eventually we will want to get rid of GILPool as well.)

[birkenfeld]

Where is this function useful? Can it do things Python::with_gil can't? If yes, then we have to accept that people are sometimes forced to use this function, in which case it being unsafe is undesirable. If no, then why do we have it?

This makes no sense to me - even if people are forced to use a function sometimes, it needs to be unsafe if we can't guarantee safety. It's better than not providing the functionality at all. FFI is not a perfect world where we can make everything safe for all use cases.

[mejrs]

This makes no sense to me - even if people are forced to use a function sometimes, it needs to be unsafe if we can't guarantee safety.

It absolutely needs to be marked unsafe if necessary.

It's better than not providing the functionality at all.

I'm concerned about:

• We deprecate this.
• An user ports over their code from using the deprecated api.
• They hit some corner case where Python::with_gil doesn't work.
• They now have to use an unsafe api to do what they could do without unsafe before.

I think it's great that users can use almost everything in pyo3 without using unsafe. If we make a core unsafe api like this that people are forced to use (or think they are forced to use) that would be a bad thing. And we should think carefully about introducing that, or introduce it in such a way that it's safe.

[birkenfeld]

I'm concerned about:
We deprecate this.
An user ports over their code from using the deprecated api.
They hit some corner case where Python::with_gil doesn't work.
They now have to use an unsafe api to do what they could do without unsafe before.

We made safety adjustments all the time in the past when it turned out that some API could lead to unsound behavior. As long as the unsafety, and the way to use the API soundly, are documented, this is fine for me.

I think it's great that users can use almost everything in pyo3 without using unsafe. If we make a core unsafe api like this that people are forced to use (or think they are forced to use) that would be a bad thing. And we should think carefully about introducing that, or introduce it in such a way that it's safe.

But what's the alternative? Remove it, making that corner case impossible?

We already have such core unsafe APIs, for example https://docs.rs/pyo3/0.14.2/pyo3/fn.with_embedded_python_interpreter.html

[mejrs]

I mean, I agree with all those things. I'm just saying that if we need to have an api like that it would be nice if it could be safe.

[davidhewitt]

An as-of-yet undiscussed alternative to removing Python::acquire_gil is that its drop order problems could be resolved by using a thread-local Vec: GilGuards could store no state themselves and instead push/pop state on the Vec.

I'm not totally sure I love that as an idea - I'm not entirely certain there aren't weird edge cases I haven't thought of, and adding another thread local would introduce a performance hit...

Aside, I'm beginning to think this PR does too much. I might split out the deoptimising Python::with_gil into a separate discussion.

[mejrs]

I'd like to get the deprecation merged and do these optimizations in the future - are there any outstanding concerns around this? Also, I've been thinking about supporting subinterpreters and I think that is much easier if Gilguard is off the board.

[davidhewitt]

I think we need to communicate the with_gil performance change very clearly (i.e. in the migration guide), however otherwise I'm happy to rebase and get this in.

Have you seen discussion in #2346? I'm getting a bit unsure about sub-interpreter support and was thinking we might need to disable it explicitly for soundness reasons.

[mejrs]

Are you working on a rebase right now? Otherwise I'll just open a new PR with these changes.

[davidhewitt]

Please go ahead. I was planning to cut this back a lot and just deprecate acquire_gil for now (not change with_gil for the moment until we also start reworking the core objects to not need the gilpool).

[mejrs]

Closing in favour of #2549