Python::allow_threads is unsound in the presence of send_wrapper.

[mejrs]

It allows smuggling Python types into the closure:

use pyo3::prelude::*;
use pyo3::types::PyString;
use send_wrapper::SendWrapper;

fn main() {
    Python::with_gil(|py| {
        let string = PyString::new(py, "foo");

        let wrapped = SendWrapper::new(string);

        py.allow_threads(|| {
            let smuggled: &PyString = *wrapped;
            println!("{:?}", smuggled);
        });
    });
}

Results in
error: process didn't exit successfully: target\debug\my_module.exe (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)

So I'm not sure whose fault it is:

• Ours, for using Send for something it wasn't meant to.
• send_wrapper's, for assuming others didn't put arbitrary restrictions on Send.

[mejrs]

One way to fix it would be to get auto_traits and negative_impls working on stable and make our own auto trait, but that seems far off.

Something we could do would be to require 'static instead, because no references to Python objects are 'static.

 pub fn allow_threads<T, F>(self, f: F) -> T
    where
        F: FnOnce() -> T + 'static,
    {
        //...
    }

This would require removing GILGuard, because leaking that is the only (safe) way to get a Python<'static>.

[davidhewitt]

Yikes, good spot! I definitely argue that it's our fault, we do slightly abuse the fact that gil-bound types don't implement Send to repurpose the auto-trait to what we want. send_wrapper is correctly protecting the invariant of the trait.

I don't think 'static is very practical, because then the closure can't capture any variables except owned values by move. I think that'll turn out quite awkward.

I think your comment #2140 (comment) is exactly right - by having a nightly feature then users for whom the Send bound is problematic can use nightly to get the result they need. And in the long term, yes I think we want to get auto traits stabilised. Or we can convince Python to drop the GIL 😅

[mejrs]

I'm thinking I'd like to use the nightly feature for this. Would it be OK to gut out specialization?

[davidhewitt]

I'd be fine with that.

[mejrs]

One thing I'm semi worried about is an interaction like this:

pyo3's lib.rs

#![feature(auto_traits, negative_impls)]

pub unsafe auto trait Foo{}

impl !Foo for Python<'_>{}
impl !Foo for PyAny{}

pub fn cant_use_non_foo<T: Foo>(_t: T){
    // ...
}

Users cargo.toml

[dependencies]
numpy = "0.14" # Old version
pyo3 = { version = "0.15", path = "../pyo3", features = ["auto-initialize", "extension-module"]}

Users code

use numpy::{array, ToPyArray, Ix1, PyArray};
use pyo3::prelude::*;

fn main() {
    Python::with_gil(|py| {
        let py_array = array![[1i64, 2], [3, 4]].to_pyarray(py);

        pyo3::cant_use_non_foo(py_array);
    })
}

This fails when it encounters two different Pythons, but doesn't seem to care about the auto trait.:

error[E0308]: mismatched types
 --> src\main.rs:6:61
  |
6 |         let py_array = array![[1i64, 2], [3, 4]].to_pyarray(py);
  |                                                             ^^ expected struct `pyo3::python::Python`, found struct `pyo3::Python`
  |
  = note: perhaps two different versions of crate `pyo3` are being used?

I really don't know enough about how dependencies are resolved and so on to tell whether there might be a problem with this approach. Can this be sound when different pyo3 versions are in use?

[davidhewitt]

I think there will be enough problems caused by conflicting pyo3 versions that users will find it very hard to compile a working program. And if their conflicting pyo3 versions are all >= 0.15, we have the links key set in Cargo.toml to prevent multiple pyo3 versions.

TLDR, I don't think you need to worry about that.

[steffahn]

With scoped-tls, even the nightly approach to have Ungil as a custom auto trait is not sufficiently sound.

This code is problematic with or without the nightly feature of pyo3.

use pyo3::prelude::*;
use pyo3::types::PyString;
use scoped_tls::scoped_thread_local;

fn main() {
    Python::with_gil(|py| {
        let string = PyString::new(py, "foo");

        scoped_thread_local!(static WRAPPED: PyString);
        WRAPPED.set(string, || {
            py.allow_threads(|| {
                WRAPPED.with(|smuggled: &PyString| {
                    println!("{:?}", smuggled);
                });
            });
        });
    });
}

Edit: Made this a separate issue:

• Python::allow_threads is unsound in the presence of scoped-tls. #3640