-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
ENH: spatial: ensure thread-safety for KDTree #20655
base: main
Are you sure you want to change the base?
Conversation
I am a bit uncertain if property |
In context of No-GIL CPython, there are several things that are not thread-safe. For example:
|
Thanks for the pointers @sturlamolden. I have the following questions and comments:
|
The issue with |
The more serious issue is that |
Now I understand everything, so, basically we should lock on |
Query functions must wait for the atomic flag set by |
Thanks for the helpful discussion @sturlamolden! |
No problem. When I wrote the code I depended on the GIL for serialization. For example it ensures that |
Actually it is already broken as |
Great! Then this PR will fix the concurrency issues in KDTree, let me add the changes and update the title |
Note that my comments refer to |
Yes, in free-threaded python when the GIL is disabled then something needs to explicitly lock access to prevent data races. If the data race happens in python-level code, then a |
We already make use of the |
Right, C-level code will need to use C-level locking primitives. |
Use the primitives in |
Also you cannot use a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am roughly -inf on the changes to _ckdtree.pyx.
Just use the threading module. The important optmisations are in the C++ code.
@sturlamolden After an internal discussion with the Python free-threaded team, it has been decided that all the free-threaded work assumes that Given this, the only potential race condition would be accessing the |
1222137
to
de4ff55
Compare
254971c
to
14a0751
Compare
Reference issue
What does this implement/fix?
This PR adds a quick check to ensure that concurrent calls to
KDTree
do not cause any unexpected errors. This is specially handy in the context of No-GIL CPython.Additional information
In principle, concurrent access to
KDTree
is allowed out-of-the-box, since there are no methods that can mutate the state of the tree besides the constructor.