Error handling by pitdicker · Pull Request #7 · dhardy/rand

pitdicker · 2017-09-30T16:51:16Z

This PR tries out some of the error handling we seem to be arriving at in the RFC tread.

For the error enum I have now:

pub enum Error {
    /// This RNG is permanently unavailable.
    NotAvailable,
    /// Needs (re)seeding. Depending on the RNG this can mean you have to wait
    /// and retry later, or you have to (re)seed it.
    EntropyLow,
    /// Failure, retry for a limited number of times.
    TemporaryFailure,
    /// Interrupted, usually by a signal from the OS.
    Interrupted,
    /// Any RNG error not part of this list.
    Other,
    #[doc(hidden)]
    __Nonexhaustive,
}

It occurred to me the variants EntropyExhausted and NetYetSeeded on the RFC thread are essentially the same, so I combined them into EntropyLow.

This commit partly undoes your work in 0a97209, I am sorry.
Maybe it is just me being to inexperienced, but the Result with two arguments looks clearer to me. I think it helps documentation-wise, as you can now click on the return type RngError in rustdoc and see the various causes. Because we have only four different methods that can return a Result, it does not really seem an ergonomic loss.

The second commit adds back fill_bytes. I gave try_fill a default implementation in terms of fill_bytes. This makes implementation of PRNGs (which usually never error) slightly simpler, as they don't have to use Result. Wrappers become slightly more complex, as they have to wrap another method.

In the third commit I tried out the new error handling with ReadRng.

Actually I was working on the OsRng implementations, and did not plan on writing this :-).
So I have not touched the code in os.rs that much yet.

pitdicker · 2017-09-30T18:15:45Z

I am slowly getting better at rebasing 😄

pitdicker · 2017-09-30T18:44:17Z

-pub struct Error;
-
-#[cfg(feature="std")]
-impl From<::std::io::Error> for Error {


I did not add this From implementation yet.
I actually had one, that would map the IO errors you could get for OsRng to the appropriate RngError.
But I am not sure it would always be correct for other RNGs implementations. It are the more exotic ones that produce errors...

Not adding one forces RNG implementations to put a little more thought into their error handling. Which might be a plus... For ReadRng the missing implementation did not really make it more difficult, and I also expect it not to for OsRng.

This sounds sensible to me.

pitdicker · 2017-09-30T18:45:18Z

    }

-    fn try_fill(&mut self, dest: &mut [u8]) -> Result<()> {
-        impls::try_fill_via_u32(self, dest)


via_u32 was a typo maybe?

dhardy · 2017-10-01T07:37:09Z

-    }
+/// Error type for random number generators.
+#[derive(Copy, PartialEq, Eq, Clone, Debug)]
+pub enum RngError {


I'd prefer the name Error personally — less repetition and matches usual naming conventions.

I thought I was following some sort of convention after reading the Error Handling Chapter in TRPL again. But Error seems to be common, and it shorter. I will change it.

Also just noticed: It should implement the ::std::error::Error trait.

dhardy · 2017-10-01T07:37:32Z

 }
-
-/// Result type (convenience type-def)
-pub type Result<T> = ::std::result::Result<T, Error>;


Why remove this?

Have you seen my reason in the PR description?

In part the reason is that I was bitten by the redefined Result type ;-).
But I don't really see a reason to have it, as we only use it in the trait definitions in a handful of places.

In a library that uses the std::result::Result, would users also import rand::Result? Or would they want to use the std one and import our rand::Error?

It seems to me (but I am inexperienced) browsing documentation and copying examples is easier without the redefine...

dhardy · 2017-10-16T08:56:07Z

+    NotAvailable,
+    /// Needs (re)seeding. Depending on the RNG this can mean you have to wait
+    /// and retry later, or you have to (re)seed it.
+    EntropyLow,


Not so keen on the doc or name for this one, and not sure if the two different things should be combined anyway — what's the advantage?

Possible doc: For PRNGs, this implies they should be (re)seeded. For external RNGs (e.g. OsRng) this implies you should wait and try again later.

Ok, I will split them

dhardy · 2017-10-16T08:58:47Z

-pub type Result<T> = ::std::result::Result<T, Error>;
+    fn cause(&self) -> Option<&::std::error::Error> {
+        None
+    }


I don't think there's any point implementing cause if it just returns None; this is the default impl.

Didn't notice that. No, than I can happily remove it

dhardy · 2017-10-16T09:08:08Z

Ok, removing the pub type Result<T> = .. bit and some of the small clean-ups I'm happy to merge now. I'm less sure about fill_bytes and Error... I guess that's something still to be decided.

pitdicker · 2017-10-16T11:11:48Z

I'm less sure about fill_bytes and Error... I guess that's something still to be decided.

Yes, the discussion has died down a bit, but I agree there are still some things open. I also have some unposted comments on my pc about it, that need finishing :-).

After playing with this Error enum a bit more, I miss that it can not return a cause. What do you think about a struct with this enum and a cause when using std, and with only the enum field for no_std?
Another try I did was storing only the OS errno as a cause in this gist. It has the advantage that it does no allocations, but can only contain this one error type.

pitdicker · 2017-10-19T19:01:49Z

Rebased and includes the changes discussed today in #9, #10 and #12.

I have not tested the new error struct much yet.

dhardy · 2017-10-20T09:46:41Z

Regarding my previous comment — any chance you can split this PR? I hate reviewing stuff this big. But also it might be better to merge the Isaac stuff first.

pitdicker · 2017-10-20T11:58:09Z

any chance you can split this PR?

I was careful to split the PR in 3 commits. Each should be the smallest change that keeps building.

It is your call. Shall I remove the second two commits, and make PR's for them when this one lands?

dhardy

Couple of minor things. Looks like two of my comments were addressed by a later commit.

dhardy · 2017-10-20T13:10:53Z

            }

-            let reader = File::open("/dev/urandom")?;
+            let reader = File::open("/dev/urandom").unwrap();


Handle this error?

dhardy · 2017-10-20T13:11:09Z

-        pub fn new() -> Result<OsRng> {
-            let reader = File::open("rand:")?;
+        pub fn new() -> Result<OsRng, Error> {
+            let reader = File::open("rand:").unwrap();


And this one

dhardy · 2017-10-20T13:11:44Z

        }
-        pub fn try_fill(&mut self, v: &mut [u8]) -> Result<()> {
            for s in v.chunks_mut(fuchsia_zircon::ZX_CPRNG_DRAW_MAX_LEN) {
+        pub fn try_fill(&mut self, v: &mut [u8]) -> Result<(), Error> {


Huh? Did this jump a line?

That is weird, will fix.

dhardy · 2017-10-20T13:13:17Z

+        match r.read(buf) {
+            Ok(0) => return Err(Error::new(ErrorKind::Unavailable, None)),
+            Ok(n) => buf = &mut mem::replace(&mut buf, &mut [])[n..],
+            Err(_) => return Err(Error::new(ErrorKind::Other, None)),


Err(e) => return Err(Error::new(ErrorKind::Other, Box::new(e)))

dhardy · 2017-10-20T13:13:45Z

-            0 => return Err(Error),
-            n => buf = &mut mem::replace(&mut buf, &mut [])[n..],
+        match r.read(buf) {
+            Ok(0) => return Err(Error::new(ErrorKind::Unavailable, None)),


Nothing you can do with the given error type, but it does feel like the cause is missing here

It is fixed in the third commit, that gives ReadRng better error handling

dhardy · 2017-10-20T13:16:14Z

-    fn try_fill(&mut self, dest: &mut [u8]) -> Result<(), Error>;
+    fn try_fill(&mut self, dest: &mut [u8]) -> Result<(), Error> {
+        Ok(self.fill_bytes(dest))
+    }


Default impl... I'd like not to have this

Maybe we can discuss it in #12? Personally I don't see a problem, but I may be missing something.

There's an argument in the RFC about this

I will remove it

pitdicker · 2017-10-20T13:29:20Z

For OsRng there is a lot of room for improvement. I was working on this in a branch, but wanted this PR to get merged first. For OsRng it is going to mean lots of changes... (not looking forward to testing it)

Also moved the `impl_uint_from_fill` macro from `os.rs` to `randcore`. I had to modify its error handling anyway, and it is shared with `OsRng`.

pitdicker · 2017-10-20T18:42:55Z

Removed the default implementation of try_fill. Is it ok if I do the OsRng parts in another PR?

dhardy · 2017-10-21T09:53:10Z

Yes, I guess so. I'll merge this. Do a merge commit rather than a rebase on your other branch please.

pitdicker force-pushed the error_enum branch from 3fa27ca to a43a3f7 Compare September 30, 2017 18:14

pitdicker commented Sep 30, 2017

View reviewed changes

dhardy reviewed Oct 1, 2017

View reviewed changes

dhardy reviewed Oct 16, 2017

View reviewed changes

pitdicker force-pushed the error_enum branch from cb35211 to 3d46567 Compare October 19, 2017 18:56

dhardy requested changes Oct 20, 2017

View reviewed changes

pitdicker added 4 commits October 20, 2017 18:31

Add Error type and ErrorKind

849f01a

Add back fill_bytes

dbbe143

Improve error handeling of ReadRng

019d9c1

Also moved the `impl_uint_from_fill` macro from `os.rs` to `randcore`. I had to modify its error handling anyway, and it is shared with `OsRng`.

Remove default implementation from try_fill

44ef65f

pitdicker force-pushed the error_enum branch from 3d46567 to 44ef65f Compare October 20, 2017 18:40

dhardy merged commit a4e9885 into dhardy:master Oct 21, 2017

pitdicker deleted the error_enum branch October 21, 2017 14:01

dhardy mentioned this pull request Oct 21, 2017

RFC: new rand-core crate, rand adaptations rust-lang/rfcs#2152

Closed

5 tasks

Conversation

pitdicker commented Sep 30, 2017 • edited by dhardy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitdicker commented Sep 30, 2017

Uh oh!

pitdicker Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhardy commented Oct 16, 2017

Uh oh!

pitdicker commented Oct 16, 2017

Uh oh!

pitdicker commented Oct 19, 2017

Uh oh!

dhardy commented Oct 20, 2017

Uh oh!

pitdicker commented Oct 20, 2017

Uh oh!

dhardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitdicker commented Oct 20, 2017

Uh oh!

pitdicker commented Oct 20, 2017

Uh oh!

dhardy commented Oct 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pitdicker commented Sep 30, 2017 •

edited by dhardy

Loading

pitdicker Sep 30, 2017 •

edited

Loading