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

Re-enable recursive class attributes #995

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Conversation

scalexm
Copy link
Contributor

@scalexm scalexm commented Jun 22, 2020

Builds on #994 (it's actually the call to PyType_Modify through a shared reference which made me realize the problem!).

Last thing to do will be to replace value: GILOnceCell<*mut ffi::PyTypeObject> with UnsafeCell<ffi::PyTypeObject> + GILOnceCell<()> as suggested by @kngwyu.

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.

Very comprehensive solution, looks great to me! Thanks!

Comment on lines +61 to +64
/// 3) if f() does not release the GIL and does not panic, it is guaranteed to be called
/// exactly once, even if multiple threads attempt to call `get_or_init`
/// 4) if f() can panic but still does not release the GIL, it may be called multiple times,
/// but it is guaranteed that f() will never be called concurrently
Copy link
Member

Choose a reason for hiding this comment

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

👍 nice

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thank you!

src/pyclass.rs Outdated Show resolved Hide resolved
});

if let Err(err) = result {
err.clone_ref(py).print(py);
Copy link
Member

@kngwyu kngwyu Jun 23, 2020

Choose a reason for hiding this comment

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

Why we need clone_ref here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only have a &Result<(), PyErr>, and it seems that PyErr::print takes self as an argument, not &self. So I used clone_ref (we are in an unlikely error path anyway), but if there is a better way I can change that.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, thanks. Then I think we need clone_ref.

@davidhewitt
Copy link
Member

@scalexm could you also please remove the CHANGELOG entry which I'd previously written about disabling class attributes (as that's now incorrect):

  • Disable #[classattr] where the class attribute is the same type as the class. (This may be re-enabled in the future; the previous implemenation was unsound.) #975

Use some kind of two-stage initialization as described in PyO3#975, by
being very cautious about when to allow the GIL to be released.
@kngwyu
Copy link
Member

kngwyu commented Jun 24, 2020

Thank you for the update.

Last thing to do will be to replace value: GILOnceCell<*mut ffi::PyTypeObject> with UnsafeCellffi::PyTypeObject + GILOnceCell<()>

Are you going to push the change to this branch?
Or can I merge this PR as is?

@scalexm
Copy link
Contributor Author

scalexm commented Jun 24, 2020

You can merge it, we can do that other change in another PR as it’s not a bug fix but rather a technical improvement.

@kngwyu kngwyu merged commit 10306bd into PyO3:master Jun 24, 2020
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.

3 participants