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

Make get_free_list API sound #1246

Merged
merged 1 commit into from
Oct 18, 2020
Merged

Make get_free_list API sound #1246

merged 1 commit into from
Oct 18, 2020

Conversation

alex
Copy link
Contributor

@alex alex commented Oct 18, 2020

No description provided.

@alex
Make get_free_list API sound
bb3b812 
Without having a Py (which ensures the GIL is held) multiple thraeds could call get_free_list concurrently.
@davidhewitt
Copy link
Member

👍 I suspect that so far we've been lucky because this code is in practice only called with the GIL held anyway. Nice catch, thanks!

@davidhewitt davidhewitt merged commit 95ed5bb into PyO3:master Oct 18, 2020
@alex alex deleted the freelist-sound branch October 18, 2020 19:43
davidhewitt
davidhewitt reviewed Oct 18, 2020
View reviewed changes
pyo3-derive-backend/src/pyclass.rs
@@ -267,7 +267,7 @@ fn impl_class(
quote! {
impl pyo3::freelist::PyClassWithFreeList for #cls {
#[inline]
fn get_free_list() -> &'static mut pyo3::freelist::FreeList<*mut pyo3::ffi::PyObject> {
Copy link
Member

@davidhewitt davidhewitt Oct 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I already merged this PR, but now I think about it should this actually return just &mut Freelist (i.e. not 'static), so that it's not possible to get a reference that outlives the GIL lifetime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yes, probably.

Copy link
Member

Choose a reason for hiding this comment

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

👍 I'm going to write a quick follow-up PR to clean this up.

@davidhewitt davidhewitt mentioned this pull request Oct 18, 2020
Merged
@kngwyu
Copy link
Member

kngwyu commented Oct 19, 2020

Since get_free_list doesn't call CPython API, I'm not sure what the motivation for this.
Or you need to override the default implementation using py?

@davidhewitt
Copy link
Member

It's fixing a thread-safety issue: this function is handing out & mut references to a global variable. Introducing the Python token ensures that no two threads can access this concurrently.

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

@davidhewitt davidhewitt davidhewitt left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@alex @davidhewitt @kngwyu