refactor(error): simplify all boxed error wrapper definition #13725
Conversation
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
src/common/src/error.rs
Outdated
| @@ -80,7 +79,8 @@ pub struct NotImplemented { | |||
| pub issue: TrackingIssue, | |||
| } | |||
|
|
|||
| #[derive(Error, Debug)] | |||
| #[derive(Error, Debug, Box)] | |||
| #[thiserror_ext(type = RwError, backtrace)] | |||
There was a problem hiding this comment.
backtrace here means that a backtrace will be generated if the inner error does not provide one.
https://docs.rs/thiserror-ext/0.0.9/src/thiserror_ext/backtrace.rs.html#26-41
/// Capture backtrace if the error does not already have one.
pub struct MaybeBacktrace(Option<Backtrace>);
impl WithBacktrace for MaybeBacktrace {
fn capture(inner: &dyn std::error::Error) -> Self {
let inner = if std::error::request_ref::<Backtrace>(inner).is_none() {
Some(Backtrace::capture())
} else {
None
};
Self(inner)
}
fn provide<'a>(&'a self, request: &mut std::error::Request<'a>) {
if let Some(backtrace) = &self.0 {
request.provide_ref(backtrace);
}
}
}
There was a problem hiding this comment.
nit: would it be better to let it be like
| #[thiserror_ext(type = RwError, backtrace)] | |
| #[thiserror_ext(boxed = RwError, backtrace)] |
since thiserror_ext does many other things besides boxing
| impl Debug for RwError { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| write!( | ||
| f, | ||
| "{}\n{}", | ||
| self.inner, | ||
| // Use inner error's backtrace by default, otherwise use the generated one in `From`. | ||
| std::error::request_ref::<Backtrace>(&self.inner).unwrap_or(&*self.backtrace) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this part the user-facing change? i.e., "As a result, we might have to review all (important) usages of error logging and replace them with thiserror_ext::Report."
Can you elaborate a bit more about the changes?
There was a problem hiding this comment.
Strictly speaking it's not as the only user-facing part is the psql interface. However, it really impacts the developing/debugging experience as we print logs for errors. 😕
There was a problem hiding this comment.
By "user-facing" I don't mean end-user, but just wanted to know what's the behavior change.
There was a problem hiding this comment.
i.e., for users of the error type. Is Display and/or Debug and/or other things changed.
There was a problem hiding this comment.
Just want to confirm that so that we can better evaluate the radius of influence
There was a problem hiding this comment.
I'm not blocking this PR if Debug is not readable now. Either change to report or impl better Debug in the future is OK to me.
But I'd like to double-confirm these (even if I feel it's the case):
Debugis loselessDisplayis not changed
If so, I think we won't have the aws-sdk surprise again.
There was a problem hiding this comment.
IIRC
anyhow'sDebugis still relatively pretty
This is true but I'm not sure whether we should adopt the same idea. anyhow::Error can be used throughout an application, so developers may get used to the debug formatting and use it everywhere.
However, not all errors are wrapped with new types in RisingWave, and developers may also log the external errors directly. That is to say, developers still have to choose different formatting based on the concrete type. In this sense, as_report can be a more unified solution.
There was a problem hiding this comment.
Displayis not changed
Yes.
Debugis loseless
I guess it's lossy. Backtrace is lost and it'll use the derived Debug impl. However I suddenly realize that the refactor does not conflict with customized Debug implementation. Let's revert it now and redo it after #13750.
There was a problem hiding this comment.
UPDATE:
Although I don't quite like it, I have introduced a flag called report_debug to forward the Debug impl of the newtype to its thiserror_ext::Report. In this case the backtrace is still retained.
risingwave/src/common/src/error.rs
Lines 82 to 84 in 10b974e
This should be considered as a temporary solution as I believe the Debug formatting of errors are abused. Let's depend on #13750 to get them reviewed.
There was a problem hiding this comment.
My point is that: in the perfect world, Debug should never be used at all! But in fact developers are not knowledgeable or careful enough, so they may misuse Debug. That's why I think Debug should contain enough information - to make it more foolproof. Not that related with the difference between application and library.
I'm exploring whether we can detect them with custom compiler lints. 😕 |
Do you want rust-lang/rust-clippy#8581? 🤪 |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #13725 +/- ##
==========================================
+ Coverage 68.21% 68.26% +0.04%
==========================================
Files 1524 1524
Lines 262499 262221 -278
==========================================
- Hits 179074 179014 -60
+ Misses 83425 83207 -218
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Follow up of #13200. Close #13215.
Use
thiserror_ext::Boxto define the boxed types. Introducethiserror_ext::Arcto wrap the error inArc, which implementsClone.Also refactor some constructors with
thiserror_ext::Construct.Important
This PR also removes all manual
Debugimplementations for these error wrappers. As explained in #13264, we should now prefer callingas_reportorto_report_stringbefore printing the error out, instead of embedding the source chain or backtrace directly inDisplayorDebugimplementation.As a result,
Debugof these error types behaves as the defaultderive, which means that it's not human-readable anymore and does not print the backtrace of inner error. Previously, they were used by developers for debugging purposes. To fix that, we might have to review all usages of error logging and replace them withthiserror_ext::Report.Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.