RFC: Add Python::with_pool as a safe alternative to GILPool usage.

[adamreichold]

The Ungil bound is required to prevent the closure from reusing the outer GIL token via captures which does give this a limited applicability compared to direct usage of GILPool, but nevertheless I think it should be safe.

Obviously only draft as this would require a deprecation cycle and an extensive update of the documentation.

[davidhewitt]

Great insight that there is potential for a safe API here. However, I'm a bit concerned that on stable this conflicts with #2141. How do you feel about making this nightly-only, where the auto trait makes it sound?

[adamreichold]

Great insight that there is potential for a safe API here. However, I'm a bit concerned that on stable this conflicts with #2141. How do you feel about making this nightly-only, where the auto trait makes it sound?

Didn't get a notification mail again...

Hhhmmm, I am not sure about making it nigthly-only. Is it in any way more unsound than with_gil currently is? I would say no. But then again, the bar is higher for adding a new API than for removing an existing one.

One thing that is in favour of adding this as a safe if technically unsound API on stable is that I think this would reduce the probability of making mistakes compared to the existing GILPool API, meaning that that API is easy to misuse whereas the send_wrapper needs a certain purposefulness or at least another layer of wrapping.

[adamreichold]

So do we add this another option or should it replace the current API? Is it nightly-only or also-stable?

[davidhewitt]

I agree this is significantly easier to use than Python::new_pool, so I am certainly in favour of replacing. I take your point that with send_wrapper interaction there's already some murky waters, however I'm reasonably sure it's us that are in the wrong with allow_threads so it feels incorrect to add a new API to stable with is "safe" despite the misuse of Send.

How do you feel about offering this on both stable and with the nightly feature, with the difference that on stable we keep it as unsafe, and on nightly it is safe?

[adamreichold]

however I'm reasonably sure it's us that are in the wrong with allow_threads

I think that is definitely the case. We are in the wrong, but we cannot make it right without either using nightly features or making the feature useless using the 'static bound.

it feels incorrect to add a new API to stable with is "safe" despite the misuse of Send.

Strictly speaking it is incorrect, but I think we might need to make room for the immaturity of the language here to avoid the perfect being the enemy of the good. Especially since from a strict point of view, Python::with_gil is just as incorrect.

How do you feel about offering this on both stable and with the nightly feature, with the difference that on stable we keep it as unsafe, and on nightly it is safe?

If we do this and deprecate GILPool::new, the ecosystem will likely start moving towards it and therefore be safe eventually. However, I feel that we will make users unhappy this way as they have the churn of changing the API, but without the benefit of dropping the unsafe code.

Hence, under these requirements, it seems better to me to just add this as a new (mostly unsafe) API without a deprecation of the existing one, possibly even targetting v0.19.x instead of v0.20.0, so that it can gain some voluntary users before we push people this way by starting a deprecation cycle.

[davidhewitt]

Sure thing, so is the agreement that in this PR we will add

impl Python<'_> {
    #[cfg(not(feature = "nightly"))]
    unsafe fn with_pool(...) { ... }

    #[cfg(feature = "nightly")]
    fn with_pool(...) { ... }

which is suitable to cherry-pick for 0.19.1 (should we have reason to create a patch release), and we can leave an issue in the backlog to deprecate Python::new_pool after a major release or two?

My reluctance to add it as "safe" on stable also comes from the hope that we can eliminate the need for the pool entirely before too much longer. In fact, I'm just going to push the prototype I have for that now so that we can discuss whether what I've been sketching out makes sense and begin iteration in that direction.

[davidhewitt]

(Corrected the above snippet to unsafe on stable, safe on nightly.)

Also posted my current branch which might be how we remove the pool in #3205

[adamreichold]

My reluctance to add it as "safe" on stable also comes from the hope that we can eliminate the need for the pool entirely before too much longer. In fact, I'm just going to push the prototype I have for that now so that we can discuss whether what I've been sketching out makes sense and begin iteration in that direction.

I think that if there is a realistic path towards removal of GILPool entirely, then we should not put people on the wrong track by emphasizing it using a new API, even if it might be an improvement. We should implement the replacement and tell people about it.

[davidhewitt]

Ok - though I think it might be quite a while before we manage to phase out the GILPool (because it's likely very breaking and needs some further design) 😅