Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrap getrandom::Error in uuid::Error for Uuid::new_v4 #502

Closed
wants to merge 1 commit into from
Closed

Wrap getrandom::Error in uuid::Error for Uuid::new_v4 #502

wants to merge 1 commit into from

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Dec 21, 2020

I'm submitting a(n) refactor

Description

Resolves the TODO on Uuid::new_v4 by wrapping the getrandom::Error in uuid::Error.

HOWEVER: I don't know if this is a good idea. There's nothing directly wrong with using Result<_, getrandom::Error> here, if we're comfortable saying that new_v4 is using getrandom to get random bytes from the OS. Wrapping it in uuid::Error requires removing the Hash impl from uuid::Error, and on top of this, getrandom::Error only implements the std Error trait if the getrandom/std feature is activated. However, there is no (current) way to activate the feature only when both v4 and std features of uuid are activated, so we're stuck either requiring v4 to imply std or to not provide the getrandom error as our error's source(). Additionally, since randomness is highly reliable, this is very unlikely to fail, and the only real valid response is to halt the program. We previously just panicked if rand's thread_rng was not available, which is (roughly) equivalent to getrandom() failing. Maintaining this behavior is no worse than static quo, easier to use, sidesteps the features issue, and avoids the breaking change. Users who absolutely need to recover in the face of errors can always construct the random bytes themselves and/or just check that getrandom() doesn't fail.

Related Issue(s)

#475, as this is a breaking TODO on the API being requested to publish.

Closes #503: alternate (personally, preferred) approach to the same TODO

@bors bors bot closed this in 97e6bf5 Dec 21, 2020
@bors bors bot closed this in #503 Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant