rand_core and error types about getrandom HOT 15 CLOSED

There is an option we could use here if we wanted: something akin to the UNIX errno, but using an AtomicPtr to store a &'static str, behind an API like

fn set_err(msg: &'static str);
fn get_last_err() -> &str;
// or, for cfg(std):
fn set_err<T: ToString>(msg: T);

This would be fully no_std compatible with minimal run-time overhead (on successful usage). There are two drawbacks:

• some extra code size / memory usage
• a more complex API, which may be a problem for inclusion in std; also we would essentially be piggy-backing this crate to define an Error type possibly used elsewhere in Rand

from getrandom.

I don't think that such extra-complexity is warranted (plus it will be even less idiomatic that NonZeroU32). On panics users by default already will get an error message specifying that it originated in getrandom. After that they'll have to look into platform specific details either way.

As was written in the rust-random/rand#715 I think in addition to msg we could add is_retryable method and RETRYABLE error constant. Also I am not sure if automatically retrying on Interrupted error is a correct behavior. For example on Linux interrupt handler could use SA_RESTART flag to continue interruptible operations without EINTR, so hard-coding retry loop may be a wrong approach.

from getrandom.

I would prefer to use a simple (non-exhaustive) error enum without any associated explicit error codes.

from getrandom.

If we can translate the source error codes appropriately, that's probably the best option.

from getrandom.

I just reviewed the error handling — mostly I like it.

Would it be sensible to use common high-part for the two error constants?

It would be nice to have some ErrorKind enum (generated on-request from the error code, including common system-specific codes), but that can be added later.

Looking at Rand's ErrorKind, two get special handling:

• Transient causes retry soon-ish; possibly we should trap io::ErrorKind::Interrupted and retry a few times?
• NotReady allows detection of blocking, which isn't relevant to the current code here (I don't see much need to add this option, but we still could add it later if necessary, perhaps via fn is_ready()).

from getrandom.

possibly we should trap io::ErrorKind::Interrupted and retry a few times?

Hm, I think we better to do it in the rand_os wrapper instead and keep getrandom as thin as possible.

It would be nice to have some ErrorKind enum

I think conversion into io::Error mostly covers the need for an additional ErrorKind.

from getrandom.

We already do repeat-on-error for RDRAND.

I think conversion into io::Error mostly covers the need for an additional ErrorKind.

Excepting that we have a couple of custom error codes.

from getrandom.

We already do repeat-on-error for RDRAND.

We follow Intel recommendations here (see section 5.2.1), IIRC in tight loop approximately each 2nd-4th execution of rdrand returns an "error", so I think it's different from Interrupted error.

from getrandom.

I think retry-on-interrupt is applicable to all users as with RDRAND? The difference is importance; it may be that interrupt errors are rare enough that no one would notice?

keep getrandom as thin as possible.

I still plan to discuss using RDRAND as a backup option. That's another discussion, but if we did do so would have some impact here.

from getrandom.

Using NonZeroU32 in the Error type implies a requirement on rustc ≥ 1.28. Once we port Rand to use this, that requirement would be implied there too. Do we want this?

Alternatively we could just use u32; would we lose much? Likely Result<(), Error> would be larger due to the missed non-zero optimisation. Is this worth it? (In the longer term, possibly so?)

from getrandom.

The WASM implementations in #10 left a few errors poorly handled (marked TODO): two cases with a static string description and two cases with a stdweb::web::error::Error type (which is apparently a reference to a JS value).

Being able to forward an error would be nice but is not essential. Safely forwarding another error requires either a tagged enum over all possible sources or an allocator, as I understand it; neither are ideal.

So what is the best option? Can a single type be sufficient for both std and no_std uses?

And can we design a better solution for rand_core and use the same type there? Should we just use the existing rand_core type here?

Perhaps the best option is to keep the current type but map each error classification to a specific constant.

from getrandom.

Perhaps the best option is to keep the current type but map each error classification to a specific constant.

I prefer this option. I think we can document error codes in the getrandom documentation and then map those codes to rand_core::Error in rand_os.

from getrandom.

How do you think we should deal with forwarded errors then?

use stdweb::web::error::Error as WebError;

        let err: WebError = js!{ return @{ result }.error }.try_into().unwrap();
        Err(UNAVAILABLE_ERROR)  // TODO: forward err

The only useful methods this type gives us come from the Debug, Display and std::error::Error implementations. I don't want to start matching against display strings, so then we can't do anything useful here.

Should we use log to print a warning?

from getrandom.

Should we use log to print a warning?

Yes, I think it will be the best option.

from getrandom.

I think we can close this issue?

from getrandom.