Add FailingReader<R:Reader> to make every Reader fail on errors

[mneumann]

In case you want to fail upon an I/O error, you can wrap every Reader with a FailingReader.

use std::io::{Reader,IoResult};

pub struct FailingReader<R> {
    priv inner: R
}

impl<R> FailingReader<R> {
    pub fn new(inner: R) -> FailingReader<R> {
        FailingReader { inner: inner }
    }
}

impl<R: Reader> Reader for FailingReader<R> {
    fn read(&mut self, buf: &mut [u8]) -> IoResult<uint> {
        let res = self.inner.read(buf);
        if res.is_err() { fail!() }
        res
    }
}

[alexcrichton]

Is this much better than calling unwrap() everywhere? The signature of read doesn't change so you'll still get warned about all the callsites you call read from. Is using the if_ok! macro too difficult?

[mneumann]

@alexcrichton : My purpose was in using iterators. I think that iterators silently ignore errors of the underlying Reader, which is not always what we want.

[huonw]

How about something that's similar to this issue (in that it wraps a reader and modifies its error behaviour) and similar to our old conditions.

High-level overview: have errors ignored when they occur, but recorded into an external user-visible handle, and so they can decide to handle errors there.

struct ErrorHandleInternals { priv error: Option<IoError> }

pub struct ErrorHandle { priv inner: *mut ErrorHandleInternals }
impl ErrorHandle {
    /// Retrieve any IO error that has occur before this point on the
    /// corresponding `ChompingReader`.
    /// This resets the error state and so an immediate second 
    /// call will return `Ok`.
    pub fn error(&mut self) -> IoResult<()> {
        Err(unsafe {(*self.inner).error.take_unwrap()})
    }
}

pub struct ChompingReader<R> {
    priv inner: R,
    priv error_handle: ~ErrorHandleInternals,

    // can't have concurrent access to `error_handle`
    priv marker: std::kinds::marker::NoSend
}

impl ChompingReader<R> {
    pub fn new(reader: R) -> (ChompingReader<R>, ErrorHandle) { ... }
}

impl<R: Reader> Reader for ChompingReader<R> {
    fn read(&mut self, buf: &mut [u8]) -> IoResult<uint> {
        match self.inner.read(buf) {
            Ok(x) => Ok(x)
            Err(e) => {
                // set an indication that an error happened
                self.error_handle.error = Some(e)
                Err(/* substitute an EOF error */)
            }
        }
    }
}

Used like

let (mut reader, mut handle) = ChompingReader::new(my_reader);

// no failure, but errors are recorded, and the loop quits immediately...
for line in reader.lines() {
    foo(line);
}
// ... so handling the error here immediately after the loop 
// is pretty similar to handling it manually with .read_line.
match handle.error() {
    Err(e) => println!("error occured in `.lines`)
    Ok(()) => {}
}

On first blush this is a lot of effort to use, and doesn't exactly seem to be worth it for things like .lines where .read_line in a loop is a perfectly good substitute. (Also, the "magic" modifications to ErrorHandle would need to be careful to be done correctly, possibly even by storing *RefCell<> instead of *mut.)

[alexcrichton]

It's also entirely possible that the Lines iterator struct could store the last error and you could match on it afterwards. There's no guarantee that you actually handled the error, but it's indeed somewhere.

[huonw]

To be sure that errors are not ignored, we could have either the Lines iterator, or the ErrorHandle have a destructor of the form (this introduces an unfortunate "surprise" failure):

fn drop(&mut self) {
    if !task::failing() {
        match *self {
            Err(err) => fail!("unhandled IO error: {}" ,err),
            Ok(_) => {}
        }
    }
}

(Possibly with an explicit .ignore_errors() method.)

[lilyball]

If Lines stores the error, that makes it a bit awkward to get at.

fn main() {
    let mut lines = io::stdin().lines();
    for line in lines {
        handle_line(line);
    }
    match lines.err {
        Ok(()) => (),
        Err(e) => handle_error(e)
    }
}

Add to that what @huonw suggested, which is that to avoid ignoring the error (which is the root cause of this issue), you'd need a destructor. Besides being ugly, this also has the problem of turning what should be a compile-time lint into an unexpected runtime failure. If I'm going to have runtime failure, I'd rather it just be triggered directly from lines() rather than be caused by me omitting error-handling code later.

At this point, I'm leaning towards saying that .lines() should simply fail!() on any non-EOF error. Then we can introduce a struct that will convert all errors into EOF, which will allow errors to be ignored by .lines(). This has the benefit that this struct can be used elsewhere with APIs that work with Readers when you explicitly don't want to handle errors, for whatever reason. It leaves the basic stdin-processing loop the same as it is today, and the behavior will more closely match scripting languages (which apparently like to raise exceptions in this same situation). The equivalent to handling the exception in the scripting language will be either ignoring errors or switching to explicit calls to .read_line(), depending on whether you want to squelch or handle the errors.

[DaGenix]

I think we should either remove lines() entirely or convert it to return an IoResult like every other io related method. Having lines() behave differently than everything else seems like it could cause issues. But that doesn't mean that we can't keep it pleasant to use.

I posted to the ML that I favored just removing it. However, thinking about it more I think that creating a wrapping iterator that fails on errors might work too. Lets say that we added something like the following to libstd:

trait IoResultIterator {
    fn fail_on_error(self) -> FailOnErrorIterator<Self> {
       FailOnErrorIterator {
           wrapped: self
       }
    }
}

impl <T, I: Iterator<IoResult<T>>> IoResultIterator for I {}

struct FailOnErrorIterator<I> {
    wrapped: I
}

impl <T, I: Iterator<IoResult<T>>> Iterator<T> for FailOnErrorIterator<I> {
    fn next(&mut self) -> Option<T> {
       match self.wrapped.next() {
           Some(x) => match x {
               Ok(y) => Some(y),
               Err(io::IoError { kind: io::EndOfFile, .. }) => None,
               _ => fail!()
           },
           None => None
       }
    }
}

Then, I think the simple "read lines from STDIN case" becomes:

fn main() {
    for line in io::stdin().lines().fail_on_error() {
        print!("received: {}", line);
    }
}

which is a little bit more verbose than it is now where it ignores errors. However, it doesn't risk surprising anyone with a failure that they didn't expect or force them to check for errors differently with lines() than anywhere else.

EDITED to say that the wrapper fails on errors.

[DaGenix]

@mneumann Does having an Iterator wrapper that fails on errors satisfy your use case (Assuming all IO related iterators are updated to return IoResult<>)?

[mneumann]

@DaGenix: Yes.

[DaGenix]

I opened up #12414 which implements my proposal. I note in that issue that this discussion is still ongoing. I opened the issue to provide some code that can be analyzed, commented on, and/or criticized.