-
Notifications
You must be signed in to change notification settings - Fork 125
feat: snafu error with better stack trace #717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
bad(for developers) error msgs: |
|
in this error, we can do similar things thanks to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
clash_lib/src/error/dns.rs:9
- The variant name 'InvaldDomain' appears misspelled. Consider renaming it to 'InvalidDomain' for clarity.
InvaldDomain {
clash_lib/src/app/dns/resolver/enhanced.rs:600
- This call to self.exchange(message).await is recursively invoking the same function, which may result in infinite recursion. Consider calling 'exchange_no_cache' or the intended non-recursive method instead.
let rv = self.exchange(message).await?;
| source: crate::error::DnsError, | ||
| }, | ||
| #[snafu(display("empty dns"))] | ||
| DsnEmpty { |
Copilot
AI
Apr 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variant name 'DsnEmpty' seems to be a typo. Consider renaming it to 'DnsEmpty' to match the intended meaning.
| DsnEmpty { | |
| DnsEmpty { |
🤔 This is a ...
🔗 Related issue link
#405
💡 Background and solution
background
greptimedb has done a great job in engineerizing error handling in rust. currently, we are in great debt of error-handlings, it's inconvenient for debugging(i had a deep feeling about that when implementing zero-copy).
so i tried a POC project in https://github.com/VendettaReborn/better-error-experiment, and it worked as expected, with NOT TOO MUCH overhead added.
And i just tried it our project, removing
anyhowin both ClashDnsResolver and Hysteria protocol implementation, i think it may serve as a draft and better POC now.solution
implement(copied from greptimedb)
stack_trace_debugmacro, which will take thelocationfield as the file location in backtrace, and will takeerrorfield as the external error field. it's important that the two field name, 'location' and 'error' is hardcoded, but that's enough for now.as you can see, with the propagate of Snafu Error, the
Locationis accumulated as a list, which will serve as the virtual error stack at the end.overhead: after defining all error kinds properly, the only overhead is adding
.context(xxxSnafu)before using?orxxxSnafu.fail()for rust-style error propagate.TODOs
crates/...☑️ Self-Check before Merge