fix races when initializing #[pyclass] type objects
#5341
Conversation
CodSpeed Performance ReportMerging #5341 will not alter performanceComparing Summary
|
davidhewitt
force-pushed
the
disallow-races
branch
from
2de7155 to
360a881
Compare
|
|
||
| if self.fully_initialized_type.get(py).is_some() { | ||
| // `tp_dict` is already filled: ok. | ||
| let Some(guard) = InitializationGuard::new(&self.initializing_thread) else { |
There was a problem hiding this comment.
Just to be sure I get the logic here:
The idea is that multiple threads might acquire a guard here, but only one of them will be able to will start the initialization due to the PyOnceLock. After that the ThreadId will be set, so any re-entrant call will be guaranteed to not get a guard and return early. On the success path the other threads will just retrieve the typeobject from the PyOnceLock and in the failure case the next thread will attempt initialization.
Is that roughly right?
There was a problem hiding this comment.
Yes, though the more I think about this I think there might be deadlock risks in other more perverse cases e.g. A and B which each have a class attribute of the other's type.
I feel like it might be that this InitializationGuard stuff is excessive and we should just ensure that only one thread ever enters this dict initialization step, I think rather than just avoid re-entrancy we might want to do something like always return the type object if any thread is currently attempting initialization.
I guess there's a secondary concern about what happens if one attribute fails to compute partway through, some attributes might be set and others not. Is that ok?
Maybe there's an argument that we should spend some more time thinking about this, get it right once in 0.27, and document the full behaviour properly at the same time.
|
@davidhewitt would you like a hand getting this rebased and updated for 0.28? |
|
Sure thing, the main thing that got it stuck was #5341 (comment) where someone needs to examine the semantics of class attribute initialization and decide what we want to guarantee. |
3 participants
This is split from #5223 as a follow-up which enacts the changes to
LazyTypeObjectto usePyOnceLock, and in the process fixes #5211Only the second commit is new, the rest is in #5223.