Error handling discussion


(John John Tedro) #21

Yes.

Panics have the benefit of the process abruptly aborting, which is why a single branch of execution is enough - that’s all there is.

Results are potentially captured and conditional logic might be executed. And I believe it’s important to as unambiguously as possible capture that context for troubleshooting.

If also reemphasize that since this is an internal API is something that can be tweaked easily as we move forward. So any decision right now is not permanent. If you don’t want to budge I don’t mind moving forward with your proposal until we encounter an example where this would be useful.


#22

This makes sense. But the first thing user would like to know after reading error message is where it is originated from. And root error backtrace will answer, not top layer backtrace.

There exist exotic cases when code tries to handle error and fails itself which creates 2 origins of error.
But it can be even more complex where code tries N approaches and returns error when all of them fail. All backtraces should be available this way.

I’m trying to make a point that user should opt-in into additional backtrace captures in complex cases.
We can make this available, but shouldn’t make it so by default.
Any default can cover only one exotic case. Let’s make default to cover simple common case instead :slight_smile:


(John John Tedro) #23

So I’ve been experimenting a bit. And only capturing backtraces in the root error is surprisingly messy since the backtrace is captured in Error::new (which is used by the From<E> where E: error::Error impl). This is necessary to capture backtraces when converting errors in simple try statements (e.g. File::open("file.txt")?).

There are a couple of solutions:

If your primary concern is about rendering “too much” information, this is up to whatever renders the error chain. It can skip over rendering any backtraces except the root cause one:

for e in e.causes().take(1) {
    println!("Error: {}", e);
}

for e in e.causes().skip(1) {
    println!("Caused by: {}", e);
}

if let Some(bt) = e.root().and_then(|e| e.backtrace()) {
    println!("Root Backtrace: {}", bt);
}

If it’s about capturing backtraces (which is kinda expensive) we need another solution that is not super straight forward. Like using a custom trait for Into<Error> for converting whatever is returned in the closure into an error to do something differently from the regular From implementation:

pub trait IntoRawError {
    fn into_raw_error(self) -> Error;
}

impl IntoRawError for Error {
    fn into_raw_error(self) -> Error {
         self
    }
}

impl<T> IntoRawError for T where T: 'static + error::Error + Send + Sync {
    fn into_raw_error(self) -> Error {
        Error::without_backtrace(self)
    }
}

The above doesn’t cover error created using e.g. Error::from_string though, I’m not sure it’s even possible to prevent that from capturing a backtrace without hindering capturing overall. I’m also not super happy with having to introduce a “special” conversion trait. If you have other suggestions feel free to propose them.

This experience is however making me more keen to punt this for a future pull request unless a clean solution can be found.


(John John Tedro) #24

@omni-viral poke? can I get your input on this?


#25

Why do you need special conversion trait?
Here is my thinking of how API should be designed regarding backtrace caputre.

  • From<E> where E: Error should capture backtrace.
  • Error::new should capture backtrace too.
  • Error::from_string should capture backtrace as well.
  • Error::with_context should take self as source and context: impl Display + 'static as context and not capture another backtrace as one is already captured. Here context is not expected to be our Error type, but some custom std::error::Error implementation or just a string or whatever that can be made into text.

Those are 4 ways to create Error instance.
Only one of them is used for chaining and it doesn’t capture backtrace while others do capture.


(John John Tedro) #26

The question here is that what if the thing in with_context does capture a backtrace (like Error::new does). Is it OK to have that overhead and discard that?

Additionally, if we want with_context to take impl Display + 'static we would have to implement Context<D> which we currently don’t. Instead we require the context to implement Into<Error>. Changing this would cause it’s own set of complexities. Do you want us to do that?


#27

Context<D> is not required. Error::with_context will return Error erasing type information of the impl Display.

impl Error {
  fn with_context(self, context: impl (Display + 'static)) -> Error {
    let mut error = Error::without_backtrace(context); // Similar to `Error::new` but doesn't capture backtrace.
    error.backtrace = Arc::clone(self.backtrace);
    error.source = Box::new(self);
    error
  }
}