Error handling: Error type

[dhardy]

This is about design of the error type; see also #8 (should we just panic?) and #9 (error kinds).

(This assumes we do return a Result somewhere and don't do all handling via panics.)

Possible error types:

• simple kind enum
• kind enum + OS code — probably not useful (see below)
• kind + cause (wrapped error) [std only]
• kind + &'static str — also not a good choice; see below

---

Rustdoc: https://dhardy.github.io/rand/rand_core/struct.Error.html

New proposal, which tries to be uniform between std and no_std (aside from missing cause), and allow documentation of both the current error and its cause without creating a new type.

#[derive(Debug)]
pub struct Error {
    pub kind: ErrorKind,
    msg: &'static str,
    #[cfg(feature="std")]
    cause: Option<Box<stdError + Send + Sync>>,
}

impl Error {
    #[cfg(feature="std")]
    pub fn new<E>(kind: ErrorKind, msg: &'static str, cause: Option<E>) -> Self
        where E: Into<Box<stdError + Send + Sync>>
    {
        Self { kind, msg, cause: cause.map(|inner| inner.into()) }
    }

    #[cfg(not(feature="std"))]
    pub fn new<E>(kind: ErrorKind, msg: &'static str, _cause: Option<E>) -> Self {
        Self { kind, msg }
    }
}

Previous proposal:

#[cfg(feature="std")]
// impls: Debug, Display, ::std::error::Error
pub struct Error {
    pub kind: ErrorKind,
    pub cause: Option<Box<std::error::Error>,
}
#[cfg(not(feature="std"))]
// impls: Debug, Display, ::std::error::Error, Clone, PartialEq, Eq
pub struct Error {
    pub kind: ErrorKind,
    pub cause: Option<&'static str>,
}

[pitdicker]

Maybe a third choice:

• kind enum + cause with std feature. Only kind enum for no_std.

They all have advantages and disadvantages. A simple kind enum is easiest to match against. But the other two options would work similar to std::io::error and error-chain, so they should be ergonomic enough to.

'kind enum + cause' can take easy part in error chaining. The disadvantage is that the error type is different in std and no_std mode.

'kind enum + OS code' can take part in error chaining, but only for errors like those of std::io::error. It can work the same in std and no_std mode, and doesn't need to allocate on an error.

'kind enum + OS code' has my preference for now. If we go with this option, I have been toying with the thought that is should maybe be a very small separate crate. It would have to reimplement small parts of std::io::error: we can't wrap ::std::error::Error::description because the function signature does not remember the description is 'static.

[dhardy]

Another thing we considered was &'static str, or perhaps kind + &'static str.

We could manually map OS codes to static strings, or do the dodgy thing I suggested before, potentially leaking memory.

Or we could do kind + cause where cause is the wrapped error in std mode and the OS code in no_std mode.

But how useful is the cause really? In theory kind alone should include enough information most of the time? I guess the answer is that provided the context of the error isn't lost, the kind alone should be enough to localise the error — but extra details might still help.

[pitdicker]

We could manually map OS codes to static strings, or do the dodgy thing I suggested before, potentially leaking memory.

I didn't think this was a serious option. It seems practical. But it is not very clean, and it would set a bad precedent for other crates. I think it would be acceptable for a small crate or a private application, bud not for something as popular as rand.

But how useful is the cause really?

For OsRng the kind NotAvailable can have as a cause that you do not have enough permission to open /dev/random, that there are not enough file descriptors, or someone deleted the device. I think mapping all these causes to NotAvailable makes sense, as that is all users of the library care about.

But for someone trying to figure out why OsRng does not work on his system, it may be useful. (If there are not countless other programs that fail on such a system ;-) ).

OsRng is of course just one thing. ReadRng, something like a USB hardware RNG, or a strange RNG that works over the network can also have OS errors that can help.

The problem is, I don't care very strongly about returning a cause. But with std::error::Error and error-chain it seems to become expected of libraries, and it may just be good to go along.

[dhardy]

Ditto, I feel like we could do without a cause, but it's probably worth including anyway.

I updated the relevant section of the RFC.

[pitdicker]

On the flip side, OsRng implementations may have to handle a std::io::Error, in which case if we don't want to throw away the details, our error type either needs to be able to hold a cause of this type or hold the string version, retrieved with type &str from std::error::Error::description.

A somewhat related question: do we want to provide OsRng with no_std? It is certainly doable, but a bit more complex than the current implementation.

[dhardy]

no_std means no standard OS — either no OS at all or something without all standard functionality. I imagine any replacement for OsRng must be domain specific. I created a no_std issue.

[pitdicker]

Than we do not really have a reason to return an OS error as cause in no_std mode. That would make the option 'kind enum + OS code' not all that interesting.

[pitdicker]

I now like your proposal in the RFC:

#[cfg(feature="std")]
pub struct Error {
    pub kind: ErrorKind,
    pub cause: Option<Box<std::error::Error>,
}
#[cfg(not(feature="std"))]
pub struct Error {
    pub kind: ErrorKind,
    pub cause: Option<&'static str>,
}

I don't like it that the error type changes, but it is practical. And that is wat no_std is about, right :-)?

For no_std the source of the &'static str is not the OS, but under our control. So it can really be a static string. Then we do not have to worry about the leaking trick.

B.t.w. is it common for the fields to be public? It sure is convenient, and I see no disadvantage.

[dhardy]

Common? Not very since it prevents internals from being changed. But I don't think these error types need to change — in fact I think it's quite useful for users to know exactly what the type is.

I'm not 100% convinced on this proposal myself but don't see anything better really.

[newpavlov]

Quite unfortunately proposed solution will not work, lets say we have no_std crate foo in which we have:

Err(rand_core::Error{kind: ErrorKind::Foo, cause: Ok("error cause")})

And now somewhere in the downstream dependency we enable std feature for rand-core, which will break foo with the following error: "expected struct alloc::boxed::Box, found reference". Plus std::error::Error is not implemented for &str, it's just that we can do (constrained) conversions using Form trait.

One solution which comes to mind is something like this:

Click to expand

#[derive(Debug)]
enum ErrorDetails {
    Static(&'static str),
    #[cfg(feature="std")]
    Error(Box<std::error::Error + Send + Sync>),
    None,
}

impl ErrorDetails {
    fn description(&self) -> &str {
        match self {
            &ErrorDetails::Static(s) => s,
            #[cfg(feature="std")]
            &ErrorDetails::Error(ref e) => e.description(),
            &ErrorDetails::None => "None", // empty string or "NA"?
        }
    }

    #[cfg(feature="std")]
    fn cause(&self) -> Option<&std::error::Error> {
        match self {
            &ErrorDetails::Error(ref e) => Some(e.as_ref()),
            _ => None,
        }
    }
}

#[derive(Debug)]
pub struct Error {
    kind: ErrorKind,
    details: ErrorDetails,
}

// method and parameter  names probably should be renamed
impl Error {
    pub fn from_kind(kind: ErrorKind) -> Self {
        Self { kind, details: ErrorDetails::None }
    }

    pub fn from_static_str(description: &'static str, kind: ErrorKind) -> Self {
        Self { kind, details: ErrorDetails::Static(description) }
    }

    #[cfg(feature="std")]
    pub fn from_error<E>(err: E, kind: ErrorKind) -> Self
        where E: Into<Box<std::error::Error + Send + Sync>>
    {
        Self { kind, details: ErrorDetails::Error(err.into()) }
    }

    pub fn kind(&self) -> ErrorKind {
        self.kind
    }

    pub fn description(&self) -> &str {
        self.details.description()
    }
}

impl fmt::Display for Error {
    ...
}

#[cfg(feature="std")]
impl std::error::Error for Error {
    ...
}

Here we hide details enum behind methods, thus removing problem of potential non-exhaustive match in no_std crate if std is enabled by someone else. Code probably can be significantly simplified if we could use #[non_exhaustive] (or #[doc(hidden)] hack), but I am not sure if it will be a good solution design wise.

P.S.: Also note additional Send + Sync constraint, which I've taken from io::Error.

[burdges]

I've forgotten my thoughts on this issue, but I'd avoid those messy method names by using impl From<T> for Error for T among ErrorKind, (ErrorKind, &'static str), and Box<std::error::Error + Send + Sync>, so that .into() works.

[newpavlov]

Hm, interesting workaround, but I am afraid it's too unorthodox. I feel we better just expose opaque wrapper around ErrorDetail enum and use fn new<T: Into<ErrorDetailWrapper>>(kind: ErrorKind, detail: T) for Error (wrapper name is subject to change), with From implemented for &'static str, Box<...> and Option<&'static str>, so users could call Error::new(kind, None).

[dhardy]

Part of the idea of the previous Error type was that it was essentially just a "kind" plus a possibly-null pointer. In this version ErrorDetails is an enum with three different values, so the null-pointer-optimisation is not enough to represent the enum.

Still, @newpavlov's point about wanting to make compatible types may be enough justification to do this. I'm just not sure whether it's better to have

struct Error {
    kind: enum { .. },
    cause: enum {
        Str(&'static str),
        #[cfg(feature="std")]
        Error(Box<std::error::Error + Send + Sync>),
        None
    },
}

as above, or

struct Error {
    kind: enum { .. },
    msg: Option<&'static str>,
    #[cfg(feature="std")]
    cause: Option<Box<std::error::Error + Send + Sync>>,
}

[newpavlov]

Box is not only (fat) pointer, but also deallocation logic which is not compatible with static strings.

I like the first type more, as it's better conveys intentions behind it. Also it has a smaller memory footprint as well. Although I am not fully sure how everything gets layouted in the memory, but I think we shouldn't care about it. :)

UPD: Unfortunately I couldn't find a way to convert &'static str variant into &std::error::Error, so I am not sure if we should use cause as a field name.

[dhardy]

Ok, PR #32 (& comments on #30) make me think some more about this.

Kind + explanation + chained cause

Error chaining is there to give a good explanation of what's going on; e.g.

Error: reseeding failed
Because: SomeSourceRng failed
Because: ...

So implementing a custom error type which can explain the current error (reseeding failed) and encapsulate the cause makes sense. The Error type from #30 doesn't allow specification of the current error and the cause without wrapping both up in a new type and calling that the cause. This seems pretty pointless.

So should we (1) revise Error to represent the kind, description and a chained cause (like my last suggestion above), or should we (2) replace Error with a trait? No, trait objects can't be returned without Box, which requires alloc or std, so that doesn't work.

Benchmark: switching gen_bytes_* to use try_fill(..).unwrap() and then adding a 2kiB payload to the Error type has no observable effect on performance, so either the error handling gets optimised out (I tried to make sure at least OsRng can genuinely fail) or large error types don't (necessarily) affect performance during normal usage.

So I think the second error type above may be better; then custom error types like this are not needed.

I'm also wondering about adding a common trait like this, to allow more uniform no_std behaviour and functions to return custom error types which don't necessarily need kind or msg fields, however without Box I'm not sure how to use it effectively:

// Don't derive std Error directly because of no_std
pub trait ErrorTrait {
    fn kind(&self) -> ErrorKind;
    fn description(&self) -> &str;
    fn cause(&self) -> Option<&ErrorTrait>;
}
#[cfg(feature="std")]
impl ::std::error::Error for ErrorTrait { .. }

Kind when chaining

Another thing #32 made me think about: what should kind mean given chained errors? Presumably, unless the error can be handled somehow (e.g. reporting Transient when a source is Unavailable then switching to an alternative source), it is still the original cause kind that should be reported? That's not what this implementation does however.

[newpavlov]

I really don't think that we should care about chaining, especially considering that error chains will be quite shallow. And introducing our own error trait feels wrong too. In other words, I think rand-core is too low-level for such kind of stuff and should be focused on RNG specific problems.

In my understanding std::error::Error already kind of supports chaining through continuous calling of cause until you'll get None and this is the way encouraged by std. I don't think it's the best solution, not the last because of the no_std issue, but it's that we have.

[dhardy]

On investigation into std::io, all uses of Error::new(kind, error) (within the io module) pass a static string as the error argument (even though this is E: Into<Box<Error+Send+Sync>>). So kind+msg appears to be good enough for them, and they never forward a cause error.

Within rand, however, there are several places we might want to forward a cause:

• if opening /dev/urandom fails — but cause may not be useful
• if a ReadRng fails (e.g. reading /dev/random) — but probably cause will be "failed to fill whole buffer" (from read_exact)
• if getrandom() (or iOS or OpenBSD equivalents) fails we have an OS code (i32)
• Fuchsia OS has a Rust error but basically it's another OS code
• if SeedFromRng::from_rng fails due to source RNG failure, it makes sense to forward a new error type + cause
• if ReseedingRng fails to reseed due to source RNG failure, it makes sense to forward a new error type + cause

Also, in rand we have to worry about no_std which std::io obviously doesn't.

[burdges]

It honestly looks like io::Error needs a redesign now that folks care more about no_std, maybe eliminate the double Boxing ir https://doc.rust-lang.org/src/std/io/error.rs.html#252, and add a Static(ErrorKind, &'static str) variant or something more along the lines of what you guys are discussing

[dhardy]

There's no double boxing there. There is unnecessary boxing (static strings must be boxed), so, yes, the Static variant could help. But only really for no_std, and I don't know how feasible porting std::io to no_std is?

What I find unnecessary is that bundling a message and a cause requires a new error type (and double boxing: Error::new(MyError::new("blah blah", cause)) results in both cause and the MyError object being boxed).

[burdges]

I think we're saying the same thing :

    fn _new(kind: ErrorKind, error: Box<error::Error+Send+Sync>) -> Error {
        Error {
            repr: Repr::Custom(Box::new(Custom { // Adds another unnecessary box
                kind,
                error,  // Because error is already a box
            }))
        }
    }

[dhardy]

Oh, no, I missed that Repr::Custom takes Box<Custom> type. Curious why they did this (to reduce the size of the Repr type maybe, but why?).

[pitdicker]

Personally I really like the design from #30.
I think #32 is the worsed case for that design.

It is a wrapper around two RNGs, and ideally should pass on the error of one, and a 'reseeding error' with the error of the other as a cause.

The alternative to #32 is to add a ReseedingFailed error to the error kinds in rand_core. Then a custom error type in that case is unnecessary.

if SeedFromRng::from_rng fails due to source RNG failure, it makes sense to forward a new error type + cause

I would make the wrapper as transparent as possible, and not wrap the error.

If it was up to me, my vote for keeping the current design from #30!

[dhardy]

By current design from #30 do you mean the one I implemented yesterday or the first one introduced by the PR? There's a significant change.

IMO the purpose of "kind" is distinctly different from description: the description is to tell users what happened, and the kind is there to guide handling. Of course, ErrorKind::ReseedingFailed could still be used to guide handling, but a little differently (it has the advantage I suppose that users can decide whether to continue without reseeding or fail when handling the error).

[Pratyush]

There is also the new failure crate that seems to have the backing of the core team. Perhaps it suffices to use that?

[newpavlov]

@Pratyush
I am not sure if we want to use it as a required dependency for rand-core. Although we could do feature-gated implementation of Fail trait for compatibility in no_std environments.

[Pratyush]

Perhaps. It might also be too early to move to that. It's nice, however, that the design of that crate includes no_std support from the beginning.

[burdges]

No, Fail is no_std but not their Error type. That's sounds not quite good enough.

[Pratyush]

From what I understand rand_core would just have to implement the Fail trait on a error enum defined in rand_core. It wouldn't have to use Error type.

[dhardy]

No, we very much need an Error type. Fail is a trait; implementations are "trait objects" which are by nature unsized; storing an unsized object currently requires an allocator, which many no_std platforms don't have. Result: we still need a very simple Sized error type of our own design.

I suppose we could still implement support for the Fail type anyway, but it wouldn't help us. It might help users, except any users using std get automatic support anyway (because failure is backwards compatible and we implement std::error::Error in this case), and without the Error type using failure is not very useful for no_std users anyway.

[newpavlov]

Approach implemented in the #34 has a big issue:

#[cfg(not(feature="std"))]
pub fn new<E>(kind: ErrorKind, msg: &'static str, _cause: Option<E>) -> Self {
    Self { kind, msg }
}

If you'll try to write Error::new(kind, "message", None) it will not work without explicit type annotation. This is one of the reasons why in the #30 different methods have been used.

[dhardy]

@newpavlov oh dear, I was premature with that. So I guess we need two functions:

fn new(kind: ErrorKind, msg: &'static str) -> Self
fn new_with_cause<E>(kind: ErrorKind, msg: &'static str, cause: E) -> Self

I pushed a commit directly (since this was fairly simple). I'm not so keen on the long name new_with_cause but I guess it'll do.

[dhardy]

Implemented