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

add OnceLockExt extension trait to help initialize OnceLock #4513

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidhewitt
Copy link
Member

Possible alternative to #4512

The idea here is to add an extension trait to provide a method for std::sync::OnceLock which helps to initialize the OnceLock without deadlocking.

For users with MSRV below 1.70 we could possibly have a once_cell feature which does the same for once_cell::sync::OnceCell.

DRAFT: needs tests and more complete documentation.

@ngoldbaum
Copy link
Contributor

Sorry for not commenting yesterday. I'm still getting my feet under me to have confident design opinions about PyO3.

There probably has to be a migration no matter what, even if it ends up just being a rename from GILOnceCell to PyOnceLock. Since the migration can't be avoided, I like that the extension trait allows use of the native rust standard library types, so I think I prefer this to #4512. After a deprecation cycle you'll be able to delete ~200 lines of code defining GILOnceCell in favor of this.

@colesbury
Copy link

There probably has to be a migration no matter what...

From reading #4512, it looked like that approach would not require users to change their code. I'd also be a bit concerned that this approach has more risk of deadlocks due to reentrancy or lock ordering issues. Once you start dealing with Python objects within a mutex or Once, those issues are a lot more likely because finalizers and weakref callbacks can call arbitrary code.

@colesbury
Copy link

It might also make sense to pursue both approaches. Make GILOnceCell thread-safe to make migration easier, but encourage people to use std::sync::OnceLock with this extension trait when appropriate.

@ngoldbaum
Copy link
Contributor

would not require users to change their code

With #4512 I think we'd need a rename at least, since GILOnceCell no longer relies on the GIL. Renames are of course a lot easier for users to handle than completely changing semantics and removing a type :)

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