Skip to content

rand_core and error types #3

Closed
Closed
rand_core and error types#3
Milestone
Initial release

Description

@dhardy
dhardy
opened on Jan 19, 2019

If we are to make this crate independent of rand_core, then we need to include an error type here.

We could simply copy the one rand_core uses; I think something simpler and equivalent on no_std may be preferable however. A quick look at the code shows that where we do include a cause, we are mostly just using an integer error code. Whether it is even worth forwarding the cause is another question.

Activity

  1. newpavlov commented on Jan 19, 2019

    @newpavlov
    newpavlov
    on Jan 19, 2019
    Member

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

  2. dhardy commented on Jan 20, 2019

    @dhardy
    dhardy
    on Jan 20, 2019
    MemberAuthor

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

  3. dhardy commented on Feb 18, 2019

    @dhardy
    dhardy
    on Feb 18, 2019
    MemberAuthor

    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()).
  4. dhardy
    added this to the Initial release milestone on Feb 18, 2019

  5. newpavlov commented on Feb 18, 2019

    @newpavlov
    newpavlov
    on Feb 18, 2019
    Member

    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.

  6. dhardy commented on Feb 18, 2019

    @dhardy
    dhardy
    on Feb 18, 2019
    MemberAuthor

    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.

  7. newpavlov commented on Feb 18, 2019

    @newpavlov
    newpavlov
    on Feb 18, 2019
    Member

    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.

  8. dhardy commented on Feb 18, 2019

    @dhardy
    dhardy
    on Feb 18, 2019
    MemberAuthor

    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.

  9. dhardy commented on Feb 22, 2019

    @dhardy
    dhardy
    on Feb 22, 2019
    MemberAuthor

    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?)

  10. dhardy commented on Feb 26, 2019

    @dhardy
    dhardy
    on Feb 26, 2019
    MemberAuthor

    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.

  11. newpavlov commented on Feb 26, 2019

    @newpavlov
    newpavlov
    on Feb 26, 2019
    Member

    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.

  12. dhardy commented on Feb 26, 2019

    @dhardy
    dhardy
    on Feb 26, 2019
    MemberAuthor

    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?

  13. newpavlov commented on Mar 4, 2019

    @newpavlov
    newpavlov
    on Mar 4, 2019
    Member

    Should we use log to print a warning?

    Yes, I think it will be the best option.

  14. dhardy commented on Mar 7, 2019

    @dhardy
    dhardy
    on Mar 7, 2019
    MemberAuthor

    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

  15. newpavlov commented on Mar 11, 2019

    @newpavlov
    newpavlov
    on Mar 11, 2019
    Member

    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.

  16. newpavlov commented on May 30, 2019

    @newpavlov
    newpavlov
    on May 30, 2019
    Member

    I think we can close this issue?

  17. dhardy
    closed this as completedon May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions