Blocking get #92
Blocking get #92
Conversation
|
cc @KodrAus, this is probably a mistake that we don't want to do in stdlib 😅 |
Turns our, real world code can has **really** subtle bugs with non-blocking semantics
|
If I understand your change, you want to make I can imagine that not making Would there be any negative consequence to this change besides it being a 'technically' breaking change? |
|
I can see that a blocking Regardless of the merits of whether I feel like the change reduces bugs for the case when threads are created in the init block, and increases bugs for the case where threads are not created. The latter case is more common. If the change is made, then we should provide a non-blocking version of |
|
Yeah, I am on the fence about this. I wouldn’t worry to much about backwards compatibility, I believe no one relies on the non-blocking guarantee, and I also now that this fixes at least one actual bug in existing code. What I am worried more is that, if get is on the hot path, we inflate code size quite a bit, as we turn a load and branch into a function call. OTOH, the hot path is most likely get_or_init anyway. I am also worried about making Clone, Debug and friends blocking. But also note that get_or_init (or, more specifically, get_or_try_init) are not an answer as well, as the problem is within the Lazy type. |
|
Which actually makes me think: perhaps we should fix only Lazy? |
|
I just double checked. Lazy is not using I am curious as to why the code which had the bug you mentioned is using |
|
The code actually uses Still, I'd like to think more about this whole situation and, in particular, what should be the semantics of |
|
Haha. Happens to me all the time :) |
|
I was looking through issues for a different reason, but this seems the most relevant. I was wondering if it would make sense to add some general handle type that would combine It seems like |
|
@RReverser good suggestion! I've opened #102, and I think we should have it. I think we don't even need cond-var, as we already have |
|
I think it is clear that we don't want to pursue this. |
…crum Add lazy initialization primitives to std Follow-up to rust-lang#68198 Current RFC: rust-lang/rfcs#2788 Rebased and fixed up a few of the dangling comments. Some notes carried over from the previous PR: - [ ] Naming. I'm ok to just roll with the `Sync` prefix like `SyncLazy` for now, but [have a personal preference for `Atomic`](rust-lang/rfcs#2788 (comment)) like `AtomicLazy`. - [x] [Poisoning](rust-lang/rfcs#2788 (comment)). It seems like there's [some regret around poisoning in other `std::sync` types that we might want to just avoid upfront for `std::lazy`, especially if that would align with a future `std::mutex` that doesn't poison](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/parking_lot.3A.3AMutex.20in.20std/near/190331199). Personally, if we're adding these types to `std::lazy` instead of `std::sync`, I'd be on-board with not worrying about poisoning in `std::lazy`, and potentially deprecating `std::sync::Once` and `lazy_static` in favour of `std::lazy` down the track if it's possible, rather than attempting to replicate their behavior. cc @Amanieu @sfackler. - [ ] [Consider making`SyncOnceCell::get` blocking](matklad/once_cell#92). There doesn't seem to be consensus in the linked PR on whether or not that's strictly better than the non-blocking variant. In general, none of these seem to be really blocking an initial unstable merge, so we could possibly kick off a FCP if y'all are happy? cc @matklad @pitdicker have I missed anything, or were there any other considerations that have come up since we last looked at this?
…crum Add lazy initialization primitives to std Follow-up to rust-lang#68198 Current RFC: rust-lang/rfcs#2788 Rebased and fixed up a few of the dangling comments. Some notes carried over from the previous PR: - [ ] Naming. I'm ok to just roll with the `Sync` prefix like `SyncLazy` for now, but [have a personal preference for `Atomic`](rust-lang/rfcs#2788 (comment)) like `AtomicLazy`. - [x] [Poisoning](rust-lang/rfcs#2788 (comment)). It seems like there's [some regret around poisoning in other `std::sync` types that we might want to just avoid upfront for `std::lazy`, especially if that would align with a future `std::mutex` that doesn't poison](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/parking_lot.3A.3AMutex.20in.20std/near/190331199). Personally, if we're adding these types to `std::lazy` instead of `std::sync`, I'd be on-board with not worrying about poisoning in `std::lazy`, and potentially deprecating `std::sync::Once` and `lazy_static` in favour of `std::lazy` down the track if it's possible, rather than attempting to replicate their behavior. cc @Amanieu @sfackler. - [ ] [Consider making`SyncOnceCell::get` blocking](matklad/once_cell#92). There doesn't seem to be consensus in the linked PR on whether or not that's strictly better than the non-blocking variant. In general, none of these seem to be really blocking an initial unstable merge, so we could possibly kick off a FCP if y'all are happy? cc @matklad @pitdicker have I missed anything, or were there any other considerations that have come up since we last looked at this?
…crum Add lazy initialization primitives to std Follow-up to rust-lang#68198 Current RFC: rust-lang/rfcs#2788 Rebased and fixed up a few of the dangling comments. Some notes carried over from the previous PR: - [ ] Naming. I'm ok to just roll with the `Sync` prefix like `SyncLazy` for now, but [have a personal preference for `Atomic`](rust-lang/rfcs#2788 (comment)) like `AtomicLazy`. - [x] [Poisoning](rust-lang/rfcs#2788 (comment)). It seems like there's [some regret around poisoning in other `std::sync` types that we might want to just avoid upfront for `std::lazy`, especially if that would align with a future `std::mutex` that doesn't poison](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/parking_lot.3A.3AMutex.20in.20std/near/190331199). Personally, if we're adding these types to `std::lazy` instead of `std::sync`, I'd be on-board with not worrying about poisoning in `std::lazy`, and potentially deprecating `std::sync::Once` and `lazy_static` in favour of `std::lazy` down the track if it's possible, rather than attempting to replicate their behavior. cc @Amanieu @sfackler. - [ ] [Consider making`SyncOnceCell::get` blocking](matklad/once_cell#92). There doesn't seem to be consensus in the linked PR on whether or not that's strictly better than the non-blocking variant. In general, none of these seem to be really blocking an initial unstable merge, so we could possibly kick off a FCP if y'all are happy? cc @matklad @pitdicker have I missed anything, or were there any other considerations that have come up since we last looked at this?
The current API guarantees that
.getnever blocks, but I think that was a mistake.Specifically, if code spawns threads in
get_or_initand the spawned thread does get.unwrap, it migth panic, while it is reasonable to expect that it'll always see the initialized value (b/c initialization started before the thread was spawned). This sounds esoteric, but I did found a well motivated instance this in a real code base.So, we should fix
.getsuch that it blocks. The docstring for.getsays that it is non-blocking, but I am willing to make a technically breaking change to the semantics. I feel that more code will be fixed by this change, and the fix does not worth causing the whole ecosystem to update Cargo.toml to 2.0.