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.

Sign up for GitHub

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

Jump to bottom

Bound 'py lifetime by GILPool in the internal usecases #869

Merged
merged 2 commits into from
Apr 13, 2020
Merged

Bound 'py lifetime by GILPool in the internal usecases #869

merged 2 commits into from
Apr 13, 2020

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Apr 12, 2020

Fixes #653.
By using GILPool::py instead of Python::assume_gil_acquired, we can inhivit &'static reference as an argument.
This PR also pushes some cleanups around POOL.

davidhewitt
davidhewitt approved these changes Apr 12, 2020
View reviewed changes
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.

Looks good to me, thanks for doing this! Got a few ideas how to simplify the code 😄

src/gil.rs Show resolved Hide resolved
src/gil.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/gil.rs Outdated Show resolved Hide resolved
src/gil.rs Show resolved Hide resolved
src/gil.rs Show resolved Hide resolved
tests/ui/static_ref.stderr Show resolved Hide resolved
@davidhewitt
Copy link
Member

Should we add a # Safety doc to Python::assume_gil_acquired() to specifically mention that 'static lifetimes can easily be inferred but are never safe? Then this PR can probably also resolve #800 for now.

@davidhewitt davidhewitt mentioned this pull request Apr 12, 2020
Closed
@kngwyu
Copy link
Member Author

kngwyu commented Apr 12, 2020 •
edited
Loading

@davidhewitt
Thank you for the comments!
I think they are all addressed now.

davidhewitt
davidhewitt approved these changes Apr 12, 2020
View reviewed changes
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.

Looks brilliant now! Thank you! I feel like the soundness holes in pyo3 are steadily closing 😄

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/python.rs
///
/// # Safety
/// The lifetime `'p` must be shorter than the period you *assume* that you have GIL.
/// I.e., `Python<'static>` is always *really* unsafe.
Copy link
Member

Choose a reason for hiding this comment

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

👍 nice. Not sure the comma is necessary.

Suggested change
/// I.e., `Python<'static>` is always *really* unsafe.
/// I.e. `Python<'static>` is always *really* unsafe.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know but it looks a matter of Blitish/American English https://english.stackexchange.com/questions/6894/should-there-be-a-comma-after-i-e

Copy link
Member

Choose a reason for hiding this comment

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

Heh that makes sense with me being British 😂

@kngwyu @davidhewitt
Apply suggestions from David
ae81865 
Co-Authored-By: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@kngwyu kngwyu merged commit d5b696f into PyO3:master Apr 13, 2020
@kngwyu kngwyu deleted the unsafe-gilpool branch April 13, 2020 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhewitt davidhewitt davidhewitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Static references to Python objects are allowed, but unsafe
2 participants
@kngwyu @davidhewitt