Make tempfile robust against TLS deallocation #281
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
No need to match result of f if it is called only once.
The fastrand Rng should be created manually to avoid TLS deallocation issues. The fastrand::alphanumeric function uses a TLS storage internally. With current Rust and fastrand implementation, this is no problem. Yet, we can speed up the process a bit by not accessing the storage for every single random char.
Honestly, I'd rather fix #178 if we're going to do anything here (although I'm still not sold on that either). |
Correct
Either that or
Since this is a hypothetical issue (I guess, because nobody complained), we can keep it as it is. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a result of discussion at smol-rs/fastrand#77.
Right now the fastrand implementation is robust with current Rust implementation against TLS deallocation issues, because the Rng struct does not require any form of drop, so the TLS memory is kept in tact. At least I was unable to provoke an issue in any way.
Even if Rust changes at some time and
mem::needs_dropstarts returningtrueforRng, the instantiation of a new Rng will succeed. Unfortunately, it won't be random at that point, because the seed is a constant value in this case. See https://github.com/smol-rs/fastrand/blob/dda0fe824b078bc844300db86021c2552465c468/src/global_rng.rs#L26 for implementation.It really depends if tempfile is actually supposed to be functional during TLS deallocation (i.e. while thread local storages are dropped). If this is no goal, this PR can simply be closed.