-
Notifications
You must be signed in to change notification settings - Fork 135
Add GILGuard::check() #269
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
Conversation
| } | ||
|
|
||
| /// Checks if the current thread holds the GIL. | ||
| #[cfg(Py_3_4)] |
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 just realized Py_3_4 doesn't exist in the main cpython crate. Is there any way to condition a cpython feature on the target version of Python? Or should I have a dummy PyGILState_Check function for Python 3.3 that is just unimplemented!()?
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've been looking through the code, and I can't find any precedence for how we should handle this, so this approach seems fine to me.
That said, Python 3.3 has been end-of-life for nearly four years, so maybe this is a suitable opportunity to drop support for it. @dgrunwald - what do you think?
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'm OK with dropping Python 3.3 (and 3.4 too).
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.
Though actually, the cfgs from python3-sys are supposed to be available in the cpython crate as well: https://github.com/dgrunwald/rust-cpython/blob/master/build.rs#L45-L46
The two build.rs files communicate via an environment variable to accomplish this.
So I'm not sure what's going wrong there.
Adds a function that returns true if the GIL is currently held for the current thread. PyGILState_Check() was only added in Python 3.4, so this doesn't support Python 2.7. When working with py.allow_threads(...) it is useful for the inner code to be able to verify the GIL is not held in case it needs to acquire the GIL on other threads. Being able to check this can help prevent accidental deadlocks. Ideally this could be done via static typing, similar to how the 'py' object ensures the GIL is held, but I couldn't think of a clever way to achieve this.
This seems inherently racy: the thread calling Does CPython expose fallible GIL APIs instead? Similar to https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.try_lock |
This function only checks if the current thread holds the lock. If it returns true then the current thread holds the lock, so no other thread can affect it, and if it returns false, then it doesn't matter if other threads acquire or release the lock, the false value is still correct. |
|
Ah I see, thanks! |
|
Thanks for merging this. Sorry for not responding about the #cfg earlier. Life got busy. |
Adds a function that returns true if the GIL is currently held for the
current thread.
When working with py.allow_threads(...) it is useful for the inner code
to be able to verify the GIL is not held in case it needs to acquire the
GIL on other threads. Being able to check this can help prevent
accidental deadlocks.
Ideally this could be done via static typing, similar to how the 'py'
object ensures the GIL is held, but I couldn't think of a clever way to
achieve this.