Skip to content

Conversation

VendettaReborn
Copy link
Contributor

🤔 This is a ...

  • Refactoring
  • Code style optimization

🔗 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 anyhow in both ClashDnsResolver and Hysteria protocol implementation, i think it may serve as a draft and better POC now.

solution

  1. use snafu instead of thiserror/anyhow
#[derive(Snafu)]
#[snafu(visibility(pub))]
#[stack_trace_debug]
pub enum DnsError {
    #[snafu(display("invalid domain: {domain}"))]
    InvaldDomain {
        domain: String,
        #[snafu(implicit)]
        location: Location,
    },
    #[snafu(display("hickory proto"))]
    Proto {
        #[snafu(implicit)]
        location: Location,
        #[snafu(source)]
        error: hickory_proto::ProtoError,
    },
}
  1. implement(copied from greptimedb) stack_trace_debug macro, which will take the location field as the file location in backtrace, and will take error field as the external error field. it's important that the two field name, 'location' and 'error' is hardcoded, but that's enough for now.

  2. as you can see, with the propagate of Snafu Error, the Location is accumulated as a list, which will serve as the virtual error stack at the end.

  3. overhead: after defining all error kinds properly, the only overhead is adding .context(xxxSnafu) before using ? or xxxSnafu.fail() for rust-style error propagate.

TODOs

  • discuss if we should adopt this error handling approach (the cost of migrating .etc)
  • move snafu stack trace related code to properer positions instead of crates/...
  • register StatusCode for each unique error type
  • migrate legacy code' error handling logic to Snafu, adjust workspace structure

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • Doc is updated/provided or not needed
  • Changelog is provided or not needed

@VendettaReborn VendettaReborn marked this pull request as draft February 14, 2025 14:26
@VendettaReborn
Copy link
Contributor Author

@VendettaReborn
Copy link
Contributor Author

in this error, we can do similar things thanks to backtrace field, but it requires error_generic_member_access feature of
compiler, and the backtrace msg is not user-friendly/readable, code:
https://gist.github.com/VendettaReborn/4876cc32aacb149b93c44bcc13c62f9d

Copy link
Contributor

@Copilot Copilot AI left a 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 {
Copy link
Preview

Copilot AI Apr 11, 2025

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.

Suggested change
DsnEmpty {
DnsEmpty {

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant