Skip to content

Conversation

@ngoldbaum
Copy link
Contributor

Partially supersedes #1109.

PyList_GetItem and PyDict_GetItem return borrowed references, which are not safe on the free-threaded build if the dict or list are possibly exposed to other threads. Python 3.13 added new variants of these APIs (PyList_GetItemRef and PyDict_GetItemRef) that return strong references and avoid the possibility of use-after-free errors due to a list or dict item getting deallocated due to the dict or list being manipulated by another thread.

To use these new APIs, I added a copy of pythoncapi_compat.h - see the pythoncapi-compat project for more details about that. I also updated the third-party licenses file with a new entry.

Let me know if you'd prefer I vendor the code using e.g. a git submodule instead of how I did it here.

@ngoldbaum
Copy link
Contributor Author

Some notes for review: PyDict_GetItem doesn't do error checking at all and will silently ignore exceptions that get triggered. This can happen in pathological situations - the easiest way to trigger it is by defining an object where __hash__ raises an exception. PyDict_GetItemRef does error checking, so I added new code paths to account for that.

I thought a little bit about races and given that dict and list are thread-safe, I think if we simply allow races to happen we don't need to add any locking in the fsevents extension. We just need to account for the possibility. That's why I added a new code path for one of the PyDict_DelItem cases to account for the possibility of another thread removing the item while the current thread is trying to remove it.

@ngoldbaum ngoldbaum force-pushed the strong-ref-apis branch 2 times, most recently from 14307cb to 16c44ab Compare September 16, 2025 20:14
@BoboTiG
Copy link
Collaborator

BoboTiG commented Sep 18, 2025

Some notes for review: PyDict_GetItem doesn't do error checking at all and will silently ignore exceptions that get triggered. This can happen in pathological situations - the easiest way to trigger it is by defining an object where __hash__ raises an exception. PyDict_GetItemRef does error checking, so I added new code paths to account for that.

I thought a little bit about races and given that dict and list are thread-safe, I think if we simply allow races to happen we don't need to add any locking in the fsevents extension. We just need to account for the possibility. That's why I added a new code path for one of the PyDict_DelItem cases to account for the possibility of another thread removing the item while the current thread is trying to remove it.

Would it be possible to write tests for that?

@ngoldbaum
Copy link
Contributor Author

Would it be possible to write tests for that?

Yes, I'm planning to add some explicitly multithreaded tests for that in a followup, after fixing the bug causing #1132. I'd also like to see how hard it is to run the tests under TSan and check that before declaring support for the free-threaded build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants