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

Revise rand_core::Error (alt #771) #800

Merged
merged 10 commits into from May 29, 2019
3 changes: 2 additions & 1 deletion rand_core/Cargo.toml
Expand Up @@ -18,10 +18,11 @@ travis-ci = { repository = "rust-random/rand" }
appveyor = { repository = "rust-random/rand" }

[features]
std = ["alloc"] # use std library; should be default but for above bug
std = ["alloc", "getrandom", "getrandom/std"] # use std library; should be default but for above bug
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this coupled to getrandom?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean we should support std without getrandom? We could try, but (1) getrandom supports (pretty-much) any std platform anyway and (2) I'm not sure Cargo allows us to imply getrandom/std only when both std and getrandom are enabled.

alloc = [] # enables Vec and Box support without std
serde1 = ["serde", "serde_derive"] # enables serde for BlockRng wrapper

[dependencies]
serde = { version = "1", optional = true }
serde_derive = { version = "^1.0.38", optional = true }
getrandom = { version = "0.1", optional = true }
168 changes: 59 additions & 109 deletions rand_core/src/error.rs
Expand Up @@ -9,169 +9,119 @@
//! Error types

use core::fmt;
#[cfg(not(feature="std"))]
use core::num::NonZeroU32;

#[cfg(feature="std")]
use std::error::Error as stdError;
#[cfg(feature="std")]
use std::io;

/// Error kind which can be matched over.
#[derive(PartialEq, Eq, Debug, Copy, Clone)]
pub enum ErrorKind {
/// Feature is not available; not recoverable.
///
/// This is the most permanent failure type and implies the error cannot be
/// resolved simply by retrying (e.g. the feature may not exist in this
/// build of the application or on the current platform).
Unavailable,
/// General failure; there may be a chance of recovery on retry.
///
/// This is the catch-all kind for errors from known and unknown sources
/// which do not have a more specific kind / handling method.
///
/// It is suggested to retry a couple of times or retry later when
/// handling; some error sources may be able to resolve themselves,
/// although this is not likely.
Unexpected,
/// A transient failure which likely can be resolved or worked around.
///
/// This error kind exists for a few specific cases where it is known that
/// the error likely can be resolved internally, but is reported anyway.
Transient,
/// Not ready yet: recommended to try again a little later.
///
/// This error kind implies the generator needs more time or needs some
/// other part of the application to do something else first before it is
/// ready for use; for example this may be used by external generators
/// which require time for initialization.
NotReady,
#[doc(hidden)]
__Nonexhaustive,
}

impl ErrorKind {
/// True if this kind of error may resolve itself on retry.
///
/// See also `should_wait()`.
pub fn should_retry(self) -> bool {
self != ErrorKind::Unavailable
}

/// True if we should retry but wait before retrying
///
/// This implies `should_retry()` is true.
pub fn should_wait(self) -> bool {
self == ErrorKind::NotReady
}

/// A description of this error kind
pub fn description(self) -> &'static str {
match self {
ErrorKind::Unavailable => "permanently unavailable",
ErrorKind::Unexpected => "unexpected failure",
ErrorKind::Transient => "transient failure",
ErrorKind::NotReady => "not ready yet",
ErrorKind::__Nonexhaustive => unreachable!(),
}
}
}

// A randomly-chosen 24-bit prefix for our codes.
#[cfg(not(feature="std"))]
pub(crate) const CODE_PREFIX: u32 = 0x517e8100;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current no_std code uses a code, but one isn't always provided. This is in line with what happens in getrandom, but may not be optimal.


/// Error type of random number generators
///
/// This is a relatively simple error type, designed for compatibility with and
/// without the Rust `std` library. It embeds a "kind" code, a message (static
/// string only), and an optional chained cause (`std` only). The `kind` and
/// `msg` fields can be accessed directly; cause can be accessed via
/// without the Rust `std` library. It embeds a message (static
/// string only), and an optional chained cause (`std` only). The
/// `msg` field can be accessed directly; cause can be accessed via
/// `std::error::Error::cause` or `Error::take_cause`. Construction can only be
/// done via `Error::new` or `Error::with_cause`.
#[derive(Debug)]
pub struct Error {
/// The error kind
pub kind: ErrorKind,
/// The error message
pub msg: &'static str,
#[cfg(feature="std")]
cause: Option<Box<stdError + Send + Sync>>,
cause: Option<Box<std::error::Error + Send + Sync>>,
#[cfg(not(feature="std"))]
code: NonZeroU32,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this still have the issue that the error types are incompatible for different features, making the std feature non-additive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, with enabled std feature NonZeroU32 code gets wrapped into ErrorCode, so public API gets only extended with new methods and no_std part of API does not change in any way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally this is the case (and the code method can no longer guarantee to have Some(NonZeroU32) result), but API changes are only additive.


impl Error {
/// Create a new instance, with specified kind and a message.
pub fn new(kind: ErrorKind, msg: &'static str) -> Self {
/// Create a new instance, with a message.
pub fn new(msg: &'static str) -> Self {
#[cfg(feature="std")] {
Error { kind, msg, cause: None }
Error { msg, cause: None }
}
#[cfg(not(feature="std"))] {
Error { kind, msg }
Error { msg, code: NonZeroU32::new(CODE_PREFIX).unwrap() }
}
}

/// Create a new instance, with specified kind, message, and a
/// chained cause.
///
/// Note: `stdError` is an alias for `std::error::Error`.
///
/// If not targetting `std` (i.e. `no_std`), this function is replaced by
/// another with the same prototype, except that there are no bounds on the
/// type `E` (because both `Box` and `stdError` are unavailable), and the
/// `cause` is ignored.
/// Create a new instance, with a message and a chained cause.
///
/// This function is only available with the `std` feature.
// NOTE: with specialisation we could support both.
#[cfg(feature="std")]
pub fn with_cause<E>(kind: ErrorKind, msg: &'static str, cause: E) -> Self
where E: Into<Box<stdError + Send + Sync>>
pub fn with_cause<E>(msg: &'static str, cause: E) -> Self
where E: Into<Box<std::error::Error + Send + Sync>>
{
Error { kind, msg, cause: Some(cause.into()) }
Error { msg, cause: Some(cause.into()) }
}

/// Create a new instance, with specified kind, message, and a
/// chained cause.
///
/// In `no_std` mode the *cause* is ignored.
/// Create a new instance, with a message and an error code.
///
/// This function is only available without the `std` feature.
#[cfg(not(feature="std"))]
pub fn with_cause<E>(kind: ErrorKind, msg: &'static str, _cause: E) -> Self {
Error { kind, msg }
pub fn with_code(msg: &'static str, code: NonZeroU32) -> Self {
Error { msg, code }
}

/// Take the cause, if any. This allows the embedded cause to be extracted.
/// This uses `Option::take`, leaving `self` with no cause.
///
/// This function is only available with the `std` feature.
#[cfg(feature="std")]
pub fn take_cause(&mut self) -> Option<Box<stdError + Send + Sync>> {
pub fn take_cause(&mut self) -> Option<Box<std::error::Error + Send + Sync>> {
self.cause.take()
}

/// Retrieve the error code.
///
/// This function is only available without the `std` feature.
#[cfg(not(feature="std"))]
pub fn code(&self) -> NonZeroU32 {
self.code
}
}

impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
#[cfg(feature="std")] {
if let Some(ref cause) = self.cause {
return write!(f, "{} ({}); cause: {}",
self.msg, self.kind.description(), cause);
return write!(f, "{}; cause: {}",
self.msg, cause);
}
}
write!(f, "{} ({})", self.msg, self.kind.description())
write!(f, "{}", self.msg)
}
}

#[cfg(feature="getrandom")]
impl From<getrandom::Error> for Error {
fn from(error: getrandom::Error) -> Self {
let msg = "getrandom error";
#[cfg(feature="std")] {
Error { msg, cause: Some(Box::new(error)) }
}
#[cfg(not(feature="std"))] {
Error { msg, code: error.code() }
}
}
}

#[cfg(feature="std")]
impl stdError for Error {
impl std::error::Error for Error {
fn description(&self) -> &str {
self.msg
}

fn cause(&self) -> Option<&stdError> {
self.cause.as_ref().map(|e| e.as_ref() as &stdError)
fn cause(&self) -> Option<&std::error::Error> {
self.cause.as_ref().map(|e| e.as_ref() as &std::error::Error)
}
}

#[cfg(feature="std")]
impl From<Error> for io::Error {
impl From<Error> for std::io::Error {
fn from(error: Error) -> Self {
use std::io::ErrorKind::*;
match error.kind {
ErrorKind::Unavailable => io::Error::new(NotFound, error),
ErrorKind::Unexpected |
ErrorKind::Transient => io::Error::new(Other, error),
ErrorKind::NotReady => io::Error::new(WouldBlock, error),
ErrorKind::__Nonexhaustive => unreachable!(),
}
std::io::Error::new(std::io::ErrorKind::Other, error)
}
}
2 changes: 1 addition & 1 deletion rand_core/src/lib.rs
Expand Up @@ -50,7 +50,7 @@ use core::ptr::copy_nonoverlapping;

#[cfg(all(feature="alloc", not(feature="std")))] use alloc::boxed::Box;

pub use error::{ErrorKind, Error};
pub use error::Error;


mod error;
Expand Down
7 changes: 4 additions & 3 deletions rand_jitter/src/error.rs
Expand Up @@ -7,7 +7,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use rand_core::{Error, ErrorKind};
use rand_core::Error;
use core::fmt;

/// An error that can occur when [`JitterRng::test_timer`] fails.
Expand Down Expand Up @@ -60,10 +60,11 @@ impl From<TimerError> for Error {
// Timer check is already quite permissive of failures so we don't
// expect false-positive failures, i.e. any error is irrecoverable.
#[cfg(feature = "std")] {
Error::with_cause(ErrorKind::Unavailable, "timer jitter failed basic quality tests", err)
Error::with_cause("timer jitter failed basic quality tests", err)
}
#[cfg(not(feature = "std"))] {
Error::new(ErrorKind::Unavailable, "timer jitter failed basic quality tests")
let _ = err;
Error::new("timer jitter failed basic quality tests")
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions rand_os/src/lib.rs
Expand Up @@ -35,13 +35,13 @@
pub use rand_core; // re-export

use getrandom::getrandom;
use rand_core::{CryptoRng, RngCore, Error, ErrorKind, impls};
use rand_core::{CryptoRng, RngCore, Error, impls};

/// A random number generator that retrieves randomness from from the
/// operating system.
///
/// This is a zero-sized struct. It can be freely constructed with `OsRng`.
///
///
/// The implementation is provided by the [getrandom] crate. Refer to
/// [getrandom] documentation for details.
///
Expand Down Expand Up @@ -88,8 +88,8 @@ impl RngCore for OsRng {
// (And why waste a system call?)
if dest.len() == 0 { return Ok(()); }

getrandom(dest).map_err(|e|
Error::with_cause(ErrorKind::Unavailable, "OsRng failed", e))
getrandom(dest)?;
Ok(())
}
}

Expand Down
9 changes: 3 additions & 6 deletions src/lib.rs
Expand Up @@ -79,8 +79,7 @@ extern crate rand_pcg;


// Re-exports from rand_core
pub use rand_core::{RngCore, CryptoRng, SeedableRng};
pub use rand_core::{ErrorKind, Error};
pub use rand_core::{RngCore, CryptoRng, SeedableRng, Error};

// Public exports
#[cfg(feature="std")] pub use rngs::thread::thread_rng;
Expand Down Expand Up @@ -276,10 +275,8 @@ pub trait Rng: RngCore {
/// On big-endian platforms this performs byte-swapping to ensure
/// portability of results from reproducible generators.
///
/// This uses [`try_fill_bytes`] internally and forwards all RNG errors. In
/// some cases errors may be resolvable; see [`ErrorKind`] and
/// documentation for the RNG in use. If you do not plan to handle these
/// errors you may prefer to use [`fill`].
/// This is identical to [`fill`] except that it uses [`try_fill_bytes`]
/// internally and forwards RNG errors.
///
/// # Example
///
Expand Down
17 changes: 5 additions & 12 deletions src/rngs/adapter/read.rs
Expand Up @@ -11,7 +11,7 @@

use std::io::Read;

use rand_core::{RngCore, Error, ErrorKind, impls};
use rand_core::{RngCore, Error, impls};


/// An RNG that reads random bytes straight from any type supporting
Expand Down Expand Up @@ -73,22 +73,15 @@ impl<R: Read> RngCore for ReadRng<R> {
fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> {
if dest.len() == 0 { return Ok(()); }
// Use `std::io::read_exact`, which retries on `ErrorKind::Interrupted`.
self.reader.read_exact(dest).map_err(|err| {
match err.kind() {
::std::io::ErrorKind::UnexpectedEof => Error::with_cause(
ErrorKind::Unavailable,
"not enough bytes available, reached end of source", err),
_ => Error::with_cause(ErrorKind::Unavailable,
"error reading from Read source", err)
}
})
self.reader.read_exact(dest).map_err(|err|
Error::with_cause("error reading from Read source", err))
}
}

#[cfg(test)]
mod test {
use super::ReadRng;
use {RngCore, ErrorKind};
use RngCore;

#[test]
fn test_reader_rng_u64() {
Expand Down Expand Up @@ -131,6 +124,6 @@ mod test {

let mut rng = ReadRng::new(&v[..]);

assert!(rng.try_fill_bytes(&mut w).err().unwrap().kind == ErrorKind::Unavailable);
assert!(rng.try_fill_bytes(&mut w).is_err());
}
}