Skip to content
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

Deprecate acquire_gil #2549

Closed
wants to merge 0 commits into from
Closed

Deprecate acquire_gil #2549

wants to merge 0 commits into from

Conversation

mejrs
Copy link
Member

@mejrs mejrs commented Aug 13, 2022

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks for keeping this thread moving forward! Just a couple of tiny thoughts and then we can ship this as part of 0.17 :)

# It is recommended to check this file in to source control so that
# everyone who runs the test benefits from these saved cases.
cc 3f6554f5f2102b81ec5751bf443502711acd649bfb5f9b131119a0015037c378 # shrinks to x = 0
cc aeb61d75c7fbb350d0f8ca1b4165defa949f29dcb3a19659b232d039cadca507 # shrinks to x = 0
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look related to this change, though I'm also not against having this file checked in.

CHANGELOG.md Outdated
Comment on lines 58 to 60
- Deprecated `Python::acquire_gil` [#2549](https://github.com/PyO3/pyo3/pull/2549). This change was made for several reasons:
- GIL drop order tracking has turned out to be [error prone](https://github.com/PyO3/pyo3/issues/1683). With a scoped API like `Python::with_gil`, these are always dropped in the correct order.
- It promotes passing and keeping the GILGuard around, which is almost always not what you actually want.
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd rather just have

Suggested change
- Deprecated `Python::acquire_gil` [#2549](https://github.com/PyO3/pyo3/pull/2549). This change was made for several reasons:
- GIL drop order tracking has turned out to be [error prone](https://github.com/PyO3/pyo3/issues/1683). With a scoped API like `Python::with_gil`, these are always dropped in the correct order.
- It promotes passing and keeping the GILGuard around, which is almost always not what you actually want.
- Deprecate `Python::acquire_gil` [#2549](https://github.com/PyO3/pyo3/pull/2549).

And then let's move the explanation of why this is deprecated onto the documentation for acquire_gil?

src/marker.rs Outdated
@@ -382,6 +382,7 @@ impl<'py> Python<'py> {
/// [`PyGILState_Ensure`]: crate::ffi::PyGILState_Ensure
/// [`auto-initialize`]: https://pyo3.rs/main/features.html#auto-initialize
#[inline]
#[deprecated(since = "0.17.0", note = "prefer Python::with_gil")]
pub fn acquire_gil() -> GILGuard {
GILGuard::acquire()
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth adding a comment here to remind us as maintainers that when we remove this API for good we can also remove the drop-order tracking from GILGuard?

@mejrs
Copy link
Member Author

mejrs commented Aug 15, 2022

I don't know how I messed this up, but I'll just open a new PR

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