Blocking get

[matklad]

The current API guarantees that .get never blocks, but I think that was a mistake.

Specifically, if code spawns threads in get_or_init and 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 .get such that it blocks. The docstring for .get says 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.

[matklad]

cc @kodraus, this is probably a mistake that we don't want to do in stdlib :sweat_smile:

[pitdicker]

If I understand your change, you want to make get block if an initialization is running, and return None when the initialization is complete?

I can imagine that not making get blocking can cause really subtle bugs such as the one you encountered (?). On the other hand it also feels a bit like asking for it when spawning threads in the initialization closure...

Would there be any negative consequence to this change besides it being a 'technically' breaking change?

[manuthambi]

I can see that a blocking get can be useful sometimes. But personally, I like .get() being non-blocking. If the user wants blocking behavior, why not always use get_or_init()?

Regardless of the merits of whether get() should be blocking, I think changing the semantics without a semver change is very bad. It will subtly break (deadlock) reentrant code written carefully based on the previous documentation. Also implementations of PartialEq, Clone, Debug will all become blocking (These can be fixed to be non-blocking of course).

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 get. Otherwise dealing with reentrancy will be much harder.

[matklad]

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.

[matklad]

Which actually makes me think: perhaps we should fix only Lazy?

[manuthambi]

I just double checked. Lazy is not using get, only get_or_init.

I am curious as to why the code which had the bug you mentioned is using get instead of get_or_init?

[matklad]

The code actually uses Lazy, which uses get_or_init, so the original code is in fact not buggy, it's just that I apparently don't know how Lazy works.

Still, I'd like to think more about this whole situation and, in particular, what should be the semantics of

cell.get_or_init(|| { cell.clone(); ... })

[manuthambi]

Haha. Happens to me all the time :)

[RReverser]

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 OnceCell and Condvar and make it possible to give a OnceCell to a different thread, and later .wait() on a handle until the OnceCell is populated.

It seems like OnceCell already has most of the necessary bits in place, but creating such wrapper requires access to internals, as Condvar can only wait on a mutex guard.

[matklad]

@RReverser good suggestion! I've opened https://github.com/matklad/once_cell/issues/102, and I think we should have it. I think we don't even need cond-var, as we already have thread::park based synchronisation internally.

[matklad]

I think it is clear that we don't want to pursue this.