Prevent dropping unsendable classes on other threads.

[adamreichold]

Continuing the discussed from #3169 (comment) and #3169 (comment):

We already have checks in place to avoid borrowing these classes on other threads but it was still possible to send them to another thread and drop them there (while holding the GIL).

This change avoids running the Drop implementation in such a case even though Python will still free the underlying memory. This might leak resources owned by the object, but it avoids undefined behaviour due to access the unsendable type from another thread.

This does assume that the object was not unsafely integrated into an intrusive data structures which still point to the now freed memory. In that case, the only recourse would be to abort the process as freeing the memory is unavoidable when the tp_dealloc slot is called. (And moving it elsewhere into a new allocation would still break any existing pointers.)

[adamreichold]

I think there's a further question about what to do with #[pyclass(unsendable)]. We do have the option of leaking because we could still move the T into a box and forget it. I think at the very least we should also use PyErr::write_unraisable to report the issue.

I am wondering whether someone might (unsafely) depend on Py<T> having a stable address on the Python heap after construction? Is that actually true for tracing GC like PyPy?

Put differently, I think we could actually just not run Drop but free the memory as anything still requiring that memory to exist would need a pointer to it anyway, e.g. some kind of intrusive data structure. But this pointer would by invalidated by moving the value into a new box.

(We could enforce boxing up-front for unsendable but that seems like a cure that is worse than the disease as !Send is often applied for performance reasons, e.g. Arc versus `Rc.)

I'm now feeling somewhat tempted to deprecate unsendable to simplify the framework and just put T: Send as a general #[pyclass] requirement in a couple releases time, if users don't report a compelling reason to keep it.

With my maintainer hat on, I whole-heartedly agree. But I actually am a user of that feature: I had simulations which were not thread safe internally for performance reasons (but parallelized by running multiple instances in multiple processes). They would never be moved between threads (or actually be used in multi-threaded programs) and I would be totally fine with aborting the process if dropping these classes on the wrong thread was attempted.

[adamreichold]

Put differently, I think we could actually just not run Drop but free the memory as anything still requiring that memory to exist would need a pointer to it anyway, e.g. some kind of intrusive data structure. But this pointer would by invalidated by moving the value into a new box.

Meaning that if we do want to give the guarantee of Py<T> being pinned in place after construction, we would have to abort instead of just moving T into a newly allocated box.

[davidhewitt]

I am wondering whether someone might (unsafely) depend on Py having a stable address on the Python heap after construction? Is that actually true for tracing GC like PyPy?

At the moment I think the nature of the Python C-API exposing objects as *mut PyObject more or less forces them to have a stable address, I believe PyPy is even forced to make objects stable to work with the C-API. This is one of the attractive features of HPy just exposing opaque handles; PyPy can implement more efficiently.

It's probably best if we don't rely on Py<T> having a stable address in case we ever want to support HPy bindings.

So I think this means we're in agreement that if the object is sent to another thread, on drop we can report it with write_unraisable and then just skip running Drop for that object?

Python will then free the memory anyway as it controls the allocation.

[adamreichold]

So I think this means we're in agreement that if the object is sent to another thread, on drop we can report it with write_unraisable and then just skip running Drop for that object?

Yes, I think this the best course of action. Please feel free to hijack this PR if you have time available. Otherwise, I will try to implement the above here eventually.

[davidhewitt]

I'll see how things shape up during the week; recently I have been able to scrap together just enough time in the evenings to comment on some of the backlog of issues and PRs in my inbox :)

[adamreichold]

I'll see how things shape up during the week; recently I have been able to scrap together just enough time in the evenings to comment on some of the backlog of issues and PRs in my inbox :)

I added the minimal implementation here and extended the test case to actually check the intended effect, i.e. that Drop is not invoked. Please have a look when there this time.

[davidhewitt]

Looks great to me, thanks! Just one thought on the test.

[davidhewitt]

💯

bors r+

[bors]

Build failed:

• conclusion

[davidhewitt]

Ah, the tests using unraisablehook only can run on 3.8 or higher. I can fixup this evening.

[adamreichold]

Added the relevant #[cfg(..)] directives.

bors r=davidhewitt

[bors]

Canceled.

[adamreichold]

And let's try again, but not on WASM...

bors retry

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• conclusion