search

smol-rs / fastrand

A simple and fast random number generator
Apache License 2.0
386 stars 33 forks source link

Remove try_with_rng #77

Closed stoeckmann closed 6 months ago

stoeckmann commented 6 months ago

The local function try_with_rng in global_rng can be removed, because it is not possible to access the thread local storage RNG twice with given functions.

Based on my understanding of the code, this would require any given function to access RNG while RNG is in use. Since this would affect every other with_rng call as well, this seems not possible.

The try_with usage exists since initial commit. My idea of this PR is to simplify the code for reviewers, because it took quite some time for me to figure out that this fixed seed usage is never reached.

At least if I'm correct. It would be great if someone could review.

notgull commented 6 months ago

The reason why try_with_RNG is used here because it isn't only possible for the TLS access to fail if it's accessed concurrently. It's also possible to fail if it's accessed during a destructor, because the destructor might be run after TLS is deallocated. I don't want Rng::new to panic during a destructor.

stoeckmann commented 6 months ago

Ah, thanks for clarification!

stoeckmann commented 6 months ago

On a second thought ...

  1. Could you clarify how a concurrent TLS access can fail, given that it's a TLS so only one thread can access RNG since it's different for every single thread? I know that it could happen if the same thread accesses RNG twice, i.e. within a with_rng call, but this does not occur here and the function is not public; not even to the crate.

  2. If you worry about a panic during TLS deallocation, isn't this also true for destructors which call bool() or any other function which effectively call with_rng?

notgull commented 6 months ago
  • Could you clarify how a concurrent TLS access can fail, given that it's a TLS so only one thread can access RNG since it's different for every single thread? I know that it could happen if the same thread accesses RNG twice, i.e. within a with_rng call, but this does not occur here and the function is not public; not even to the crate.

Sorry, I forgot to clarify, concurrent TLS access cannot fail. Only TLS deallocation can fail.

  • If you worry about a panic during TLS deallocation, isn't this also true for destructors which call bool() or any other function which effectively call with_rng?

Because there really isn't anything else we can do. We can fall back to a fixed insecure seed for an RNG, but we can't return a fixed value for the other values.

stoeckmann commented 6 months ago

Okay, that makes sense. I have compared the existing solution with other crates just to get a better understanding of the matter.

At first I was about to recommend a documentation adjustment, but then I think it's good as it is.

Thanks again for your feedback!