Skip to content

gh-128182: Add per-object memory access synchronization to ctypes #128490

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

Merged
merged 24 commits into from
Jan 13, 2025

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Jan 4, 2025

I say "per-object" as in, using a single ctypes object is thread-safe if that's the only object accessing the memory. IMO, access to arbitrary memory across different pointer objects shouldn't (or can't?) be thread safe--ctypes should remain pretty low level, and that kind of synchronization might do bad things to applications that don't want it. I've clarified this in the docs. That said, I could see use in some sort of wrapper that ensures only one thread accesses memory using a table of addresses, but that should go to DPO first.

A few notes about the implementation here:

  • I'm making some assumptions about ctypes casting getfunc and setfunc functions being non-reentrant. In practice, they really shouldn't be, but of course I'm not absolutely certain. We might want to go with a recursive mutex if that turns out to be an issue.
  • I'm also assuming that the PyFoo_FromBar functions don't have a chance to re-enter. The cases I could think of that could maybe cause re-entrancy are: an embedder could hijack the allocator and run the interpreter from it, or audit events. (Let me know if this is a deal-breaker!)

📚 Documentation preview 📚: https://cpython-previews--128490.org.readthedocs.build/en/128490/library/ctypes.html#thread-safety-without-the-gil

@ZeroIntensity ZeroIntensity added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 6, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit bdee2b2 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 6, 2025
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

For locked_deref, you might want to add an additional macro with an explicit return type instead instead of casting to a void **, say locked_deref_t(T, self). I didn't count how many usages of locked_deref need casting though so you might also inline those casts.

Now, AFAICT, most of the changes can be written within a critical section because, except perhaps some of them where the lock depends on something else? (I don't really use critical sections but AFAIK, critical sections are roughly equivalent to acquire the GIL and hence safe lock all current resources, right?)

Is a critical section an overkill compared to a per-object lock as well?

Thread Safety Without The GIL
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

In Python 3.13, the :term:`GIL` may be disabled on :term:`experimental free threaded <free threading>` builds.
Copy link
Member

Choose a reason for hiding this comment

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

This is orthogonal but whenever I review FT PRs, we always use :term:`free threaded `. I believe we can add a new term for free threaded which points to free threading so that we can reduce the docs burden.

Copy link
Member Author

Choose a reason for hiding this comment

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

A free threaded term sounds like a good idea, but that should probably be in a larger follow-up PR. I'll play around with the wording here though.

@ZeroIntensity
Copy link
Member Author

Thanks for the thorough review!

For locked_deref, you might want to add an additional macro with an explicit return type instead instead of casting to a void **, say locked_deref_t(T, self). I didn't count how many usages of locked_deref need casting though so you might also inline those casts.

Acknowledged. I'll look at the cases.

Now, AFAICT, most of the changes can be written within a critical section because, except perhaps some of them where the lock depends on something else? (I don't really use critical sections but AFAIK, critical sections are roughly equivalent to acquire the GIL and hence safe lock all current resources, right?)

Critical sections are just a per-object locking mechanism, but they're safe against lock-ordering and re-entrancy deadlocks, because a thread's active locks are released when the thread state is detached (generally in a Py_BEGIN_ALLOW_THREADS block).

I could try and do this with a critical section, but it significantly complicates the change, because nearly every function that touches b_ptr will need a wrapper function that surrounds it with the critical section. I don't think that a critical section is worth the hassle, because all of the cases here are not re-entrant (at least under normal circumstances). If we're really worried about re-entrancy in something like PyBytes_FromStringAndSize, I think the better solution would be to create a copy of the pointer while the lock is still held, release the lock, and then call it with the new copy.

@ZeroIntensity ZeroIntensity requested a review from encukou January 8, 2025 21:45
ZeroIntensity and others added 4 commits January 9, 2025 10:42
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@ZeroIntensity ZeroIntensity requested a review from encukou January 10, 2025 00:18
Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@encukou encukou merged commit 8dfc743 into python:main Jan 13, 2025
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants