error-handling: simplify boxed error definition

[BugenZhao]

Background: #11443.

thiserror is good and self-contained until we have to customize it, among of which definining a wrapper of the error enum is the most frequent one.

risingwave/src/stream/src/error.rs

Lines 30 to 45 in c7eada9

	#[derive(thiserror::Error)]
	#[error("{inner}")]
	pub struct StreamError {
	inner: Box<Inner>,
	}
	#[derive(thiserror::Error, Debug)]
	#[error("{kind}")]
	struct Inner {
	#[from]
	kind: ErrorKind,
	backtrace: Backtrace,
	}
	#[derive(thiserror::Error, Debug)]
	enum ErrorKind {

The reason for wrapping is...

1. We want to capture backtrace in one go, but we don't want to add it to every variant, which is verbose and, more importantly, significantly increases the stack size of the enum!
2. We want to reduce the stack size of the enum. Returning a large result will lead to performance issue (perf: reduce Error type size #11263), so we may box it after constructing.

However, wrapping makes the interface of thiserror not that user-friendly as before.

1. All #[from] in the enum does not work any more. There's an extra conversion step from the ExternalError to ErrorInner then Error.

   • Developers may implement the conversion manually.

     risingwave/src/stream/src/error.rs

     Lines 101 to 106 in c7eada9

     	// Build expression error; ...
     	impl From<ExprError> for StreamError {
     	fn from(error: ExprError) -> Self {
     	ErrorKind::Expression(error).into()
     	}
     	}

   • What is even more concerning is that developers are not giving enough attention to thiserror annotations like #[from] and #[source], since they seem useless. However, they're really critical since they help us to maintain the source chain of an error.

     risingwave/src/common/src/error.rs

     Lines 98 to 99 in c7eada9

     	#[error("Array error: {0}")]
     	ArrayError(ArrayError),

2. Constructing an error is more painful.

   • Developers either need to write a lot of boilerplates when constructing every error,

     risingwave/src/frontend/src/binder/delete.rs

     Lines 77 to 79 in c7eada9

     	return Err(RwError::from(ErrorCode::BindError(
     	"`RETURNING` clause is not supported for tables with generated columns".to_string(),
     	)));

   • ...or define constructors manually on the wrapper.

     risingwave/src/stream/src/executor/error.rs

     Lines 91 to 98 in c7eada9

     	impl StreamExecutorError {
     	fn serde_error(error: impl Error) -> Self {
     	ErrorKind::SerdeError(error.into()).into()
     	}
     	pub fn channel_closed(name: impl Into<String>) -> Self {
     	ErrorKind::ChannelClosed(name.into()).into()
     	}

   • This seems not a big deal but actually affect the willingness for developers to code fine-grained error handling a lot. For example, I cannot find a pattern of extending an inner error with some upper-layer context in the whole codebase, which is the key design of snafu.

As the result, in #11275, we choose another way to reduce the stack size by boxing each variant separately. However, this still breaks From.

Then I'm planned to simplify this procedure with proc-macros. See linked PR for details.

[xxchan]

Ohhhhh, so basically, you are just summarize some current practice into a paradigm (with ergonomic macros), right? You are not introducing new patterns.

[xxchan]

Let me summarize my understanding, and some questions included:

(Some of) Our current practice is enum ErrorCode + backtrace, put in a box (maybe only enum is boxed like, RwError, maybe an inner struct is boxed like StreamError, but no much difference?).

The core idea is we added a layer of indirection.

Benefits are:

• reduce stack size
• capture backtrace in one place
  • Q1: why adding backtrace to every variant can increase stack size? if we box the enum
  • Q2: It seems thiserror-ext doesn't do this now?

Limitations are:

• #[from] doesn't work.
  • Q3: How is this problem solved? It seems thiserror-ext just does impl<E:Into<ErrorInner>> From<E> for Error. (so macro is not super necessary? We can manually do that?)
• Construction is more verbose because of ErrorCode::Variant(error.into()).into(). thiserror-ext solves this by auto generate the constructor functions.

---

comments:

I feel the Box macro is not super-necessary (good-to-have though), as the expanded code isn't very long and nothing special introduced. The Construct is helpful. (But I imagine it would be harder if we only want to add Construct instead of generate Box at the same time)

BTW, I also feel the pattern is kind of similar to a "statically-typed" version of anyhow::Error (with dyn error replaced to an enum). Don't know if there's any deeper correlations...🤔️

[BugenZhao]

You know me really well! 😄

why adding backtrace to every variant can increase stack size? if we box the enum

You're right. The key motivation is still to box it. Ability to add backtrace is just a by-product.

It seems thiserror-ext doesn't do this now?

Yes. Always adding a backtrace may not be the best practice since...

• if some inner error has already provide the backtrace, we should't shadow it.
• if the error occurs at the row level, it will be too costly.

I'll add the support with an option provided in the future.

• It seems thiserror-ext just does impl<E:Into<ErrorInner>> From<E> for Error.

Didn't think you notice that. 😄 Yeah, it's really easy to reuse all From implementation. Not sure why we always manually do them one by one.

thiserror-ext solves this by auto generate the constructor functions.

Not only that, there'll also be some extensions traits generated for Result<T, SourceError>. Stay tuned. 😄

(But I imagine it would be harder if we only want to add Construct instead of generate Box at the same time)

Yeah. Since the constructors should be generated on the public wrapped type, I find Construct and Box are working together seamlessly.

BTW, I also feel the pattern is kind of similar to a "statically-typed" version of anyhow::Error (with dyn error replaced to an enum). Don't know if there's any deeper correlations...🤔️

You're pretty correct. However, the benefits of static type could be:

• able to match, of course
• easier to construct, just specify from and to and the Display implementation will generate the boilerplate like "cannot cast .. to .."
• force the developers to categorize the error and keep it tidy (hopefully)

[xxchan]

Thanks for your reply! Pretty clear. Glad I understood it correctly.