add sync::OnceExt and sync::OnceLockExt traits
#4676
Conversation
That's OK. Single-initialization comes up a lot so we should take our time and get it right. I also want to add a section to the guide about how to use threads from both Python and rust and how free-threading changes things. I may expand the existing parallelism docs if needed. Having some time to do that is nice! |
| // Some other thread filled this Once, so we need to restore the GIL state. | ||
| unsafe { ffi::PyEval_RestoreThread(ts) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
I wrote a test for this trait based on the standard library example for try_once_force and it crashed Python because the thread state was corrupted. I think this code won't restore the thread state correctly if a panic happens inside f() and the fix is to use the RAII guard pattern. Assuming that's right, I'll go ahead and push a fix along with the test.
There was a problem hiding this comment.
Does the guard fix your problem? That'd be surprising to me, because I reviewed the previous implementation and believed it was sound.
There was a problem hiding this comment.
Oh nevermind; I read the Once documentation and if it is poisoned it will panic before the closure is called.
|
I looked at including |
We do? https://github.com/PyO3/pyo3/blob/main/pyo3-build-config/src/lib.rs#L138 |
|
Sorry, missed that! Thanks for the link. |
ngoldbaum
changed the title
sync::OnceExt traitsync::OnceExt and sync::OnceLockExt traits
I think I did it right, but clippy complains that I'm using features that aren't in the MSRV. OK to ignore that or did I mess something up? |
| }); | ||
| }); | ||
| assert_eq!(cell.get(), Some(&12345)); | ||
| } |
There was a problem hiding this comment.
suggestions welcome for a more interesting test
| }); | ||
| # } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I probably want to rewrite this example to use OnceLock
That's annoying. You can set it to allow here: https://github.com/PyO3/pyo3/blob/main/Cargo.toml#L153 |
|
I just locally ignored it, I still think that clippy lint is useful and it doesn't look like we use features that aren't in the MSRV very often. |
|
Thanks, this looks great to me! I simplified the |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
The new tests that use threads need to be skipped on emscripten. |
Related to #4513, #4671
It turns out that within our test suite the racy behaviour of
GILOnceCellis already problematic on GIL-enabled builds, and is just waiting for additional GIL switches to bait out problems 😬. In particular intest_declarative_modulewe have a race which might attempt to create a module twice; we explicitly forbid this because we don't support subinterpreters.This PR introduces
pyo3::sync::OnceExt, which adds helper methods tostd::sync::Oncewhich avoid deadlocking with the GIL by releasing if necessary before blocking.This solves the issue with
test_declarative_moduleby using theOnceto guarantee we only ever create the module once.I think we should document this in the FAQ and freethreading guides, and probably this is good justification for getting #4513 and maybe also some
OnceCellExttraits into 0.23. cc @ngoldbaum, I guess this would delay the release by a few days.