Repository: BurntSushi/rust-csv (language: rust)

Issue:
 Recovering from CSV read errors leads to infinite loops when caused by IO errors
#### What version of the `csv` crate are you using?
1.1.3

#### Briefly describe the question, bug or feature request.

Recovering from CSV errors leads to infinite loops when caused by IO errors

#### Include a complete program demonstrating a problem.

```rust
use {
    csv::Reader,
    std::io::{self, Read},
};

#[derive(Debug, serde_derive::Deserialize)]
struct Deserialized {
    something: String,
}

fn main() {
    for record_result in Reader::from_reader(FailingRead).deserialize::<Deserialized>() {
        match record_result {
            Ok(deserialized) => println!("Got something: {}", deserialized.something),
            Err(failed_line) => eprintln!("This record is broken :("),
        }
    }
}

struct FailingRead;
impl Read for FailingRead {
    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
        Err(io::Error::new(io::ErrorKind::Other, "reader just broke!"))
    }
}
```

#### What is the observed behavior of the code above?

Infinite loop of "This record is broken :(".

#### What is the expected or desired behavior of the code above?

```
This record is broken :(
```
*program exits*


Discussion / hints:
Hi again.
@BurntSushi This has just cost us thousands of euros due to generating tons of useless logs after an invalid file was transferred, leading to an always-erroring reader.
Would you be open to a PR fixing this ?
I don't really understand what needs to be fixed in the csv crate here. You're getting an error and then explicitly continuing. If you don't want to continue, then stop the loop when an error occurs.
We want to keep parsing other lines when there's a broken record in the actual CSV, as it's very common the CSVs we handle are broken in some way (e.g. on a single line, not all fields set, broken escapes leading to too many fields, failed to deserialize because of wrong content...), so we need to iterate on all the records, handling lines that are not broken and logging the errors: this is what this example code is meant to do (though this is a much reduced example: in practice for us it's actually hundreds of lines of code generic on iterators of results, with multiple conversion steps, `csv` not being the only library we parse from).

The issue is that when the underlying reader breaks, [`csv::DeserializeRecordsIntoIter`](https://docs.rs/csv/1.1.3/csv/struct.DeserializeRecordsIntoIter.html) becomes an infinite iterator of IO errors, and I beleive it should not, as this creates issues when handling possibly partly broken CSVs, and I can't think of a scenario where the infinite iterator of IO errors would be what somebody who still attempts to read the next record after they encountered an error on one record would want.

I'm thinking about e.g. using some kind of wrapper around the reader, like this, in order to avoid this issue:
```rust
/// Wrapper around reader to simulate an "End of file" after we hit an error
///
/// This is so that users of the reader who recover from some errors (e.g. users of csv reader) don't infinite loop.
/// "End of file" in Rust is represented by `read` returning `Ok(0)`
pub struct PretendEofAfterError<R: Read> {
	reader: R,
	has_errored: bool,
}

impl<R: Read> PretendEofAfterError<R> {
	pub fn new(reader: R) -> Self {
		Self {
			reader,
			has_errored: false,
		}
	}
}

impl<R: Read> Read for PretendEofAfterError<R> {
	fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
		if self.has_errored {
			Ok(0)
		} else {
			let res = self.reader.read(buf);
			if res.is_err() {
				self.has_errored = true;
			}
			res
		}
	}
}
```

(This is the workaround I implemented after opening this issue, but forgetting to use it in one place is what eventually cost us thousands of euros.)
Thanks for elaborating. That's a much better bug report. There is some subtlety to this though. It sounds like the iterator's behavior needs to depend on the kind of error that occurred. If an I/O error occurs, then yeah, probably the iterator should not yield anything else. But if a deserialization error occurs, then the CSV reader should advance to the next record. The latter behavior exists today:

```rust
#[derive(Debug, serde::Deserialize)]
struct Record {
    field1: String,
    field2: i32,
}

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let mut rdr = csv::Reader::from_path("weird.csv")?;
    for result in rdr.deserialize::<Record>() {
        match result {
            Ok(record) => println!("OK: {:?}", record),
            Err(err) => println!("ERROR: {:?}", err),
        }
    }
    Ok(())
}
```

```toml
[package]
name = "csv-207"
version = "0.1.0"
authors = ["Andrew Gallant <jamslam@gmail.com>"]
edition = "2018"

[dependencies]
csv = "1"
serde = { version = "1", features = ["derive"] }
```

```
field1,field2
foo,2
bar,2.5
baz,3
```

```
$ cargo run
   Compiling csv-207 v0.1.0 (/tmp/csv-207)
    Finished dev [unoptimized + debuginfo] target(s) in 0.48s
     Running `target/debug/csv-207`
OK: Record { field1: "foo", field2: 2 }
ERROR: Error(Deserialize { pos: Some(Position { byte: 20, line: 3, record: 2 }), err: DeserializeError { field: Some(1), kind: ParseInt(ParseIntError { kind: InvalidDigit }) } })
OK: Record { field1: "baz", field2: 3 }
```

I guess this is what you're saying: the "continuing" behavior of errors is correct for some kinds of errors but not for other kinds. That distinction wasn't obvious to me.

But yeah, I agree with you, if an iterator hits an I/O error, then the iterator should be marked as exhausted. And this should be clearly documented. It shouldn't be too hard to implement, but would be tedious probably. I don't know when I'll get around to it, so if you wanted to submit a PR, that would be great.

Fix patch:
diff --git a/src/reader.rs b/src/reader.rs
index dce47b43..da532659 100644
--- a/src/reader.rs
+++ b/src/reader.rs
@@ -686,6 +686,10 @@ impl ReaderBuilder {
 ///   [`ReaderBuilder`](struct.ReaderBuilder.html).
 /// * When reading CSV data from a resource (like a file), it is possible for
 ///   reading from the underlying resource to fail. This will return an error.
+///   For subsequent calls to the `Reader` after encountering a such error
+///   (unless `seek` is used), it will behave as if end of file had been
+///   reached, in order to avoid running into infinite loops when still
+///   attempting to read the next record when one has errored.
 /// * When reading CSV data into `String` or `&str` fields (e.g., via a
 ///   [`StringRecord`](struct.StringRecord.html)), UTF-8 is strictly
 ///   enforced. If CSV data is invalid UTF-8, then an error is returned. If
@@ -741,7 +745,34 @@ struct ReaderState {
     /// Whether the reader has been seeked or not.
     seeked: bool,
     /// Whether EOF of the underlying reader has been reached or not.
-    eof: bool,
+    ///
+    /// IO errors on the underlying reader will be considered as an EOF for
+    /// subsequent read attempts, as it would be incorrect to keep on trying
+    /// to read when the underlying reader has broken.
+    ///
+    /// For clarity, having the best `Debug` impl and in case they need to be
+    /// treated differently at some point, we store whether the `EOF` is
+    /// considered because an actual EOF happened, or because we encoundered
+    /// an IO error.
+    /// This has no additional runtime cost.
+    eof: ReaderEofState,
+}
+
+/// Whether EOF of the underlying reader has been reached or not.
+///
+/// IO errors on the underlying reader will be considered as an EOF for
+/// subsequent read attempts, as it would be incorrect to keep on trying
+/// to read when the underlying reader has broken.
+///
+/// For clarity, having the best `Debug` impl and in case they need to be
+/// treated differently at some point, we store whether the `EOF` is
+/// considered because an actual EOF happened, or because we encoundered
+/// an IO error
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum ReaderEofState {
+    NotEof,
+    Eof,
+    IOError,
 }
 
 /// Headers encapsulates any data associated with the headers of CSV data.
@@ -798,7 +829,7 @@ impl<R: io::Read> Reader<R> {
                 cur_pos: Position::new(),
                 first: false,
                 seeked: false,
-                eof: false,
+                eof: ReaderEofState::NotEof,
             },
         }
     }
@@ -1598,13 +1629,17 @@ impl<R: io::Read> Reader<R> {
 
         record.clear();
         record.set_position(Some(self.state.cur_pos.clone()));
-        if self.state.eof {
+        if self.state.eof != ReaderEofState::NotEof {
             return Ok(false);
         }
         let (mut outlen, mut endlen) = (0, 0);
         loop {
             let (res, nin, nout, nend) = {
-                let input = self.rdr.fill_buf()?;
+                let input_res = self.rdr.fill_buf();
+                if input_res.is_err() {
+                    self.state.eof = ReaderEofState::IOError;
+                }
+                let input = input_res?;
                 let (fields, ends) = record.as_parts();
                 self.core.read_record(
                     input,
@@ -1636,7 +1671,7 @@ impl<R: io::Read> Reader<R> {
                     return Ok(true);
                 }
                 End => {
-                    self.state.eof = true;
+                    self.state.eof = ReaderEofState::Eof;
                     return Ok(false);
                 }
             }
@@ -1716,7 +1751,7 @@ impl<R: io::Read> Reader<R> {
     /// }
     /// ```
     pub fn is_done(&self) -> bool {
-        self.state.eof
+        self.state.eof != ReaderEofState::NotEof
     }
 
     /// Returns true if and only if this reader has been configured to
@@ -1817,7 +1852,7 @@ impl<R: io::Read + io::Seek> Reader<R> {
         self.core.reset();
         self.core.set_line(pos.line());
         self.state.cur_pos = pos;
-        self.state.eof = false;
+        self.state.eof = ReaderEofState::NotEof;
         Ok(())
     }
 
@@ -1845,7 +1880,7 @@ impl<R: io::Read + io::Seek> Reader<R> {
         self.core.reset();
         self.core.set_line(pos.line());
         self.state.cur_pos = pos;
-        self.state.eof = false;
+        self.state.eof = ReaderEofState::NotEof;
         Ok(())
     }
 }


Test patch:
diff --git a/tests/tests.rs b/tests/tests.rs
index 35000994..ba739544 100644
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -1,7 +1,9 @@
 #![allow(dead_code)]
 
+use csv::Reader;
+
 use std::env;
-use std::io::{self, Write};
+use std::io::{self, Read, Write};
 use std::path::PathBuf;
 use std::process::{self, Command};
 
@@ -338,6 +340,23 @@ fn tutorial_perf_core_01() {
     assert_eq!(out.stdout(), "11\n");
 }
 
+#[test]
+fn no_infinite_loop_on_io_errors() {
+    struct FailingRead;
+    impl Read for FailingRead {
+        fn read(&mut self, _buf: &mut [u8]) -> io::Result<usize> {
+            Err(io::Error::new(io::ErrorKind::Other, "Broken reader"))
+        }
+    }
+
+    let mut record_results = Reader::from_reader(FailingRead).into_records();
+    let first_result = record_results.next();
+    assert!(
+        matches!(&first_result, Some(Err(e)) if matches!(e.kind(), csv::ErrorKind::Io(_)))
+    );
+    assert!(record_results.next().is_none());
+}
+
 // Helper functions follow.
 
 /// Return the target/debug directory path.