Add error handling to iterators

[adaszko]

Before this, iterators would simply stop (i.e. produce None) on an IO error for instance. With this PR, a proper error is produced.

[paulhauner]

Looks good to me.

In the future we'll have to keep an eye out and make sure we don't let people do collect::<Vec<Result<_, _>, _>> so they don't end up in an infinite loop. I can't think of a way to protect against this at the type-level. This risk seems like a reasonable cost in order to ensure we don't suppress DB errors.

[michaelsproul]

Nice work!

Just a few minor changes requested from me.

Regarding Paul's point about infinite iteration, I think we discussed this previously and decided that collect will short-circuit on the first Err?

[michaelsproul]

It might be cool if we could abstract this in an iterator extension trait like find_result(|slot| *slot == target_slot), but I'm happy to leave that for a future PR. Similarly for take_while.

[adaszko]

I think hiding Err(_) => true behind a function call is going to lead to problems. true within find() signifies the value has been found and Err(_) almost never is the searched value.

Attempting abstracting from a slightly different angle can produce slightly cleaner code. That angle would be itertools::process_results. For instance, translating the piece of code above:

use itertools::process_results;

pub fn root_at_slot(&self, target_slot: Slot) -> Result<Option<Hash256>, Error> {
    process_results(self.rev_iter_state_roots()?, |mut iter| {
        iter.find(|(_, slot)| *slot == target_slot).map(|(root, _)| root)
    })
}

It has the advantage that you can write your iterators pipeline as if there were no Errs in it and you get rid of the .transpose() call. The assumptions that's made is that the pipeline gets 'folded' into a single value, and not into another iterator.

Another example with take_while() for comparison:

Before:

let state_root = self
    .rev_iter_state_roots()?
    .take_while(|result| match result {
        Ok((_, current_slot)) => *current_slot >= slot,
        Err(_) => true,
    })
    .find(|result| match result {
        Ok((_, current_slot)) => *current_slot == slot,
        Err(_) => true,
    })
    .transpose()?
    .map(|(root, _slot)| root)
    .ok_or_else(|| Error::NoStateForSlot(slot))?;

After:

let state_root = process_results(self.rev_iter_state_roots()?, |iter| {
    iter.take_while(|(_, current_slot)| *current_slot >= slot)
        .find(|(_, current_slot)| *current_slot == slot)
        .map(|(root, _slot)| root)
})?.ok_or_else(|| Error::NoStateForSlot(slot))?;

[michaelsproul]

Could just be Ok(Self { values })

[adaszko]

You dislike having unnecessary local variables but they do come in handy during debugging, where you generally can't evaluate arbitrary Rust expression to see what's going on. I've applied your remark nonetheless.

[michaelsproul]

That's true, I normally refactor during debugging, or use dbg!

[adaszko]

Regarding Paul's point about infinite iteration, I think we discussed this previously and decided that collect will short-circuit on the first Err?

That's right! @paulhauner #1132 (comment)

[adaszko]

FYI, those currently failing tests pass for me on macOS. I will have to run them on my Linux machine.

[paulhauner]

Regarding Paul's point about infinite iteration, I think we discussed this previously and decided that collect will short-circuit on the first Err?

It depends on the type that you collect into. See this playground for an example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=27415619b28bdad42c3f70f573561da5

[AgeManning]

I've had a skim over this and it looks fine to me. I'll defer a more thorough review to @paulhauner and @michaelsproul who know these sections of code better.

[michaelsproul]

A beautiful application of process_results, it's perfect for this! And it addresses the short-circuit dilemma as well!

Happy to merge once conflicts are addressed 👍