New error handling + type #225

dhardy · 2018-01-08T12:47:11Z

This brings in the new error type and handling from dhardy/master. See dhardy#10.

It also adds try_fill_bytes; see dhardy#12 and dhardy#27. (There's still a question on naming here.)

fill_via_try_fill includes some code for implicit error handling; see dhardy#25.

Finally, support for NaCl is removed from OsRng; see dhardy#19.

Note that for now all Rng methods besides next_u32 have default implementations; this may change before 0.5 but not in this PR.

Also note that the new error module is deliberately private; probably it will move to rand-core, however the names publically exported in rand will still be exported there.

pitdicker

Looks good! I had some minor comments.

I see you basically copied the entire os.rs file, and created a new error module. Nice.

pitdicker · 2018-01-08T13:07:38Z

src/impls.rs

@@ -167,4 +167,40 @@ pub fn next_u64_via_fill<R: Rng+?Sized>(rng: &mut R) -> u64 {
    impl_uint_from_fill!(rng, u64, 8)
 }

+/// Implement `fill_bytes` via `try_fill` with implicit error handling.
+pub fn fill_via_try_fill<R: Rng+?Sized>(rng: &mut R, dest: &mut [u8]) {


I still feel a bit uneasy about this function. We had to pick some defaults for how many times to retry or how long to wait in case of an error, that may not suite every RNG.

What do you think about moving it to os.rs and not making it public?

At the moment it's not public. I see your point though, we could simply force every use-case to have its own custom implementation instead of having this one. I'm not sure.

pitdicker · 2018-01-08T13:10:18Z

src/lib.rs

+    /// [`fill_bytes`]: trait.Rng.html#method.fill_bytes
+    fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> {
+        Ok(self.fill_bytes(dest))
+    }


Just as a test I removed the default implementation, and you did not miss to implement it for any wrappers etc. 👍.

I would like to see the default implementations be removed, but good to wait until all the changes for RNG implementations are merged.

pitdicker · 2018-01-08T13:12:09Z

src/lib.rs

+    #[inline]
+    fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> {
+        (**self).try_fill_bytes(dest)
+    }
 }

 #[cfg(feature="std")]


Not part of this PR, but should this also be available with the alloc feature?

With that I mean impl<R: ?Sized> Rng for Box<R> where R: Rng {

Ah. Probably. Make a separate PR?

Okay. But I should probably wait until this PR has landed because they will conflict?

pitdicker · 2018-01-08T13:14:44Z

src/os.rs

-}
+    fn next_u32(&mut self) -> u32 {
+        // note: `next_u32_via_fill` does a byte-swap on big-endian
+        // architectures, which is not really needed here


I think it was me who wrote this comment? I don't really think it matters, the system call and OS RNG should dwarf the time the byte swap takes.

I thought it was me. But yes, I don't think it's important, so the comment could be removed.

On that topic, impl_uint_from_fill does not call .to_le(). It should, surely?

He, your right! While the documentations says it uses little-endian order. Really need some tests there 😄. Edit: someday though

pitdicker · 2018-01-08T13:18:52Z

src/read.rs

@@ -46,34 +47,24 @@ impl<R: Read> ReadRng<R> {

 impl<R: Read> Rng for ReadRng<R> {
    fn next_u32(&mut self) -> u32 {
-        // This is designed for speed: reading a LE integer on a LE
-        // platform just involves blitting the bytes into the memory
-        // of the u32, similarly for BE on BE; avoiding byteswapping.


This is probably a breaking change on big-endian? I don't think this will be a problem for anyone, but maybe good to mention.

Yes, but only because it wasn't portable before. I doubt anyone was depending on the old non-portable behaviour.

pitdicker · 2018-01-08T13:21:35Z

src/os.rs

-                    panic!("kern.arandom sysctl failed! (returned {}, s.len() {}, oldlenp {})",
-                           ret, s.len(), s_len);
+                    // Old format string:
+                    // "kern.arandom sysctl failed! (returned {}, s.len() {}, oldlenp {})"


Do we care about this?

pitdicker · 2018-01-08T14:03:37Z

It also adds try_fill_bytes; see dhardy#12 and dhardy#27. (There's still a question on naming here.)

I think I would go with renaming.

For RNG implementations we are going to have breaking changes anyway, so no problem in renaming there. For users of fill_bytes they would have to rename to either fill or try_fill. And I see some use in that: code that now has no choice but to panic in case of an error, is now reminded it can switch to try_fill thanks to a compiler error.

Related thought: it would be great if we had some function that could fill slices of any type. Like the from_rng code for ISAAC and ~~HC-128~~ Xorshift do for integers, and like the current implementation of Rand for arrays of other types.

dhardy · 2018-01-08T15:02:38Z

I think I agree about the renaming fill_bytes → fill forcing users to update.

Not sure what you mean about filling slices?

Travis points out something about Rustc 1.15, though very easy to work around:

error: struct field shorthands are unstable

pitdicker · 2018-01-08T17:20:23Z

src/lib.rs

+    /// Fill `dest` entirely with random data.
+    ///
+    /// Implementations of this trait must implement at least one of
+    /// `next_u32`, `next_u64`, `fill` and `try_fill` directly. In the case this


I don't think this is true, only next_u32 has to be implemented? But this part of the documentation is not going to live long anyway.

pitdicker · 2018-01-08T17:20:26Z

src/lib.rs

+    /// This is the only method which allows an RNG to report errors while
+    /// generating random data; other methods either handle the error
+    /// internally or panic. This method is intended to allow error-handling
+    /// when seeding keys or PRNGs from external (true) RNGs.


Sorry for being picky... "This method is intended to" sounds a bit specific. Maybe "This method can be used to"?

It is specific, and intended to explain why there isn't try_next_u64 etc. You think it's over-specific? I mean it's obvious there may be other ways to use it, but this is the only important one I see.

That is a good intention, useful to add. Could you reword it a little, because I missed it?
Something like

This method is intended to be the way to use external (true) RNGs, like OsRng. You primarily want to use it to seed keys or (infallible) PRNGs.

Okay, done. I'll rebase later, to let you review changes now.

I am not sure what there is left to rebase or review?

pitdicker · 2018-01-08T17:59:08Z

Not sure what you mean about filling slices?

It was just a thought. Can we provide a function that can fill slices of any size, not just arrays of at most 32 elements as gen does now? And when the slice contains just integers, can that make use of fill, instead of generating integers one by one?

I have something working, but not very ergonomic: https://gist.github.com/pitdicker/ba4c08bf407c04a6f2a63a8116afd4d1

Don't let it hold up this PR though.

dhardy · 2018-01-08T18:02:17Z

Interesting idea. Nothing to do with this PR though as I see it.

pitdicker · 2018-01-08T18:11:50Z

Having a method that can fill slices of multiple types could be an argument to rename fill_bytes or not. We could point to it as a maybe more ergonomic alternative to make the breakage a little easier to accept. Or it could be an argument not to rename fill_bytes to fill, because that name would be better for a generic method.

But I like the rename here and think it is the right choice. I will not distract this PR further.

dhardy · 2018-01-09T10:32:22Z

Sorry, I see what you mean now: replace fill with a generic function filling any slice type.

I don't know. The reason we switched key types to byte arrays was to avoid Endianness issues. I guess we could even go back to u32 / u64 arrays and do all the conversions in a fill function. I'm not so sure about it though.

pitdicker · 2018-01-09T11:15:32Z

I am really all for keeping the Rng trait as we have now, and to not complicate the fill method for RNG implementations.

I think a more generic method can be useful for rand. But at least my try depends on the distributions to provide default implementations for floats and bool.

And the only overhead for integer slices of having the u8 Rng::fill method in between something more generic is having an unaligned copy instead of an aligned copy if directly filling is not possible.

dhardy · 2018-01-09T11:31:08Z

A generic fill also feels out of place here: Rng is about what RNG sources need to provide; Sample: Rng and Distribution are about generating various types from an RNG.

Of course, you can't generated an unsized type like a slice directly, so maybe Sample should have a fill function in addition to gen? In that case it would make more sense to use fill_bytes here.

Design: #10

dhardy · 2018-01-09T11:49:59Z

Rebased, but I think I'll remove the last two renaming commits; now I've re-read the comments I see @burdges had the same idea back in October. It also avoids unnecessary breakage.

pitdicker · 2018-01-09T11:54:14Z

We can still have fill_slice as a possible name for a more generic function. You have changed your mind about the argument that it helps users to pick between a fallible and infallible method?

dhardy · 2018-01-09T12:10:00Z

To some extent, yes, because the way most people handle errors is not much better than unwrap() and worse than the implicit handling we have for some OsRng errors.

BTW I revised that handling as you suggested (see last commit).

pitdicker · 2018-01-09T12:18:09Z

I hope users of try_fill/try_fill_bytes will pass on the error with ?. It should not generally be part of some inner loop like next_u32 or next_u64. But I will leave the decision to you 😄.

Then the only open thing left is the endianness in impl_uint_from_fill, or do you want to do that as part of another PR?

dhardy · 2018-01-09T13:16:11Z

I have a TODO note on the Endianness. It wasn't introduced in this PR so I won't fix here.

Since we deliberately report NotReady errors just passing the error on with ? isn't always appropriate.

pitdicker · 2018-01-09T15:45:19Z

Feel free to merge (from me) when you think it is the right time.

This removes support for the NaCl target, which appears to be dead.

New error handling + type

dhardy mentioned this pull request Jan 8, 2018

Port rand to CloudABI. #224

Merged

pitdicker reviewed Jan 8, 2018

View reviewed changes

pitdicker mentioned this pull request Jan 9, 2018

Implement Rng for Box with alloc feature #227

Merged

dhardy added 3 commits January 9, 2018 11:38

Add new error types from dhardy/master branch

1136240

Design: #10

Fix for Rust 1.15

a68cb14

Add Rng::try_fill_bytes; revise error handling in ReadRng

230b225

dhardy force-pushed the error branch from 8ec3e5a to fb3a2c7 Compare January 9, 2018 11:46

dhardy force-pushed the error branch from 764f31c to 3874a50 Compare January 9, 2018 16:55

dhardy added 2 commits January 9, 2018 17:00

OsRng: merge in changes from dhardy/master: better error handling, -nacl

fed7943

This removes support for the NaCl target, which appears to be dead.

Inline fill_via_try_fill implicit error handling, and revise

c9bfa3a

dhardy force-pushed the error branch from 3874a50 to c9bfa3a Compare January 9, 2018 17:00

dhardy merged commit 8e8bb6a into rust-random:master Jan 10, 2018

dhardy deleted the error branch January 10, 2018 11:08

This was referenced Jan 12, 2018

Rename try_fill or fill_bytes? dhardy/rand#27

Closed

Tracker: planned changes for 0.5 #232

Closed

This was referenced Mar 11, 2018

Error handling: error sources / kinds dhardy/rand#9

Closed

Error handling: Error type dhardy/rand#10

Closed

Constructing an PRNG from fresh entropy dhardy/rand#17

Closed

Seeding PRNGs (traits and bit size) dhardy/rand#18

Closed

pitdicker mentioned this pull request Mar 23, 2018

Use SecRandomCopyBytes on MacOS #322

Merged

pitdicker pushed a commit that referenced this pull request Apr 4, 2018

Merge pull request #225 from dhardy/error

2b53718

New error handling + type

dhardy mentioned this pull request Mar 25, 2024

Make RngCore infallible? #1418

Closed

New error handling + type #225

New error handling + type #225

Conversation

dhardy commented Jan 8, 2018 • edited Loading

pitdicker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented Jan 8, 2018 • edited Loading

dhardy commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented Jan 8, 2018

dhardy commented Jan 8, 2018

pitdicker commented Jan 8, 2018

dhardy commented Jan 9, 2018

pitdicker commented Jan 9, 2018

dhardy commented Jan 9, 2018

dhardy commented Jan 9, 2018

pitdicker commented Jan 9, 2018

dhardy commented Jan 9, 2018

pitdicker commented Jan 9, 2018

dhardy commented Jan 9, 2018

pitdicker commented Jan 9, 2018

dhardy commented Jan 8, 2018 •

edited

Loading

pitdicker Jan 8, 2018 •

edited

Loading

pitdicker commented Jan 8, 2018 •

edited

Loading