search

gamedig / rust-gamedig

Game Server Query Library.
https://crates.io/crates/gamedig
MIT License
38 stars 11 forks source link

Add rich error type #80

Closed Douile closed 1 year ago

Douile commented 1 year ago

A first draft of implementing a better error type that propagates source errors and messages.

Requires bumping MSRV to 1.65.0 as it uses backtraces.

I didn't rename GDError as that would require a lot more changes throughout the codebase, however I changed GDResult to use the new GDRichError (which uses GDError as its kind part) and added a From implementation so GDErrors that are not updated are implicitly converted to the new GDRichError (with an added backtrace).

Other than that its a pretty basic type but has a custom debug formatter so an unwrapped error output looks like this:

$ RUST_BACKTRACE=1 cargo run --example generic mc-java mc.hypixel.net
Querying 209.222.115.42 with game Game {
    name: "Minecraft (java)",
    default_port: 25565,
    protocol: Minecraft(
        Some(
            Java,
        ),
    ),
}.
Get varint 0
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: PacketUnderflow
"Size requested 1 was larger than remaining bytes 0"
Backtrace [
    { fn: "gamedig::errors::GDRichError::new", file: "./src/errors.rs", line: 102 },
    { fn: "gamedig::errors::GDRichError::packet_underflow", file: "./src/errors.rs", line: 110 },
    { fn: "gamedig::errors::GDRichError::packet_underflow_from_into", file: "./src/errors.rs", line: 121 },
    { fn: "gamedig::buffer::Buffer<B>::read", file: "./src/buffer.rs", line: 110 },
    { fn: "gamedig::protocols::minecraft::types::get_varint", file: "./src/protocols/minecraft/types.rs", line: 191 },
    { fn: "gamedig::protocols::minecraft::protocol::java::Java::receive", file: "./src/protocols/minecraft/protocol/java.rs", line: 53 },
    { fn: "gamedig::protocols::minecraft::protocol::java::Java::get_info", file: "./src/protocols/minecraft/protocol/java.rs", line: 89 },
    { fn: "gamedig::protocols::minecraft::protocol::java::Java::query", file: "./src/protocols/minecraft/protocol/java.rs", line: 148 },
    { fn: "gamedig::protocols::minecraft::protocol::query_java", file: "./src/protocols/minecraft/protocol/mod.rs", line: 46 },
    { fn: "gamedig::games::query_with_timeout", file: "./src/games/mod.rs", line: 162 },
    { fn: "generic::generic_query", file: "./examples/generic.rs", line: 23 },
    { fn: "generic::main", file: "./examples/generic.rs", line: 46 },
    { fn: "core::ops::function::FnOnce::call_once", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/ops/function.rs", line: 250 },
    { fn: "std::sys_common::backtrace::__rust_begin_short_backtrace", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/sys_common/backtrace.rs", line: 135 },
    { fn: "std::rt::lang_start::{{closure}}", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/rt.rs", line: 166 },
    { fn: "core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/ops/function.rs", line: 284 },
    { fn: "std::panicking::try::do_call", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panicking.rs", line: 500 },
    { fn: "std::panicking::try", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panicking.rs", line: 464 },
    { fn: "std::panic::catch_unwind", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panic.rs", line: 142 },
    { fn: "std::rt::lang_start_internal::{{closure}}", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/rt.rs", line: 148 },
    { fn: "std::panicking::try::do_call", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panicking.rs", line: 500 },
    { fn: "std::panicking::try", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panicking.rs", line: 464 },
    { fn: "std::panic::catch_unwind", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panic.rs", line: 142 },
    { fn: "std::rt::lang_start_internal", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/rt.rs", line: 148 },
    { fn: "std::rt::lang_start", file: "/rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/rt.rs", line: 165 },
    { fn: "main" },
    { fn: "__libc_start_main" },
    { fn: "_start" },
]
', examples/generic.rs:46:59
stack backtrace:
   0: rust_begin_unwind
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_fmt
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/panicking.rs:67:14
   2: core::result::unwrap_failed
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/result.rs:1651:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/result.rs:1076:23
   4: generic::main
             at ./examples/generic.rs:46:9
   5: core::ops::function::FnOnce::call_once
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

I need to look into whether implicitly capturing backtraces whenever an error is created (errors are created whenever they are used in or_err etc.) affects performance, although that may be a non-issue as backtraces are only captured if the RUST_BACKTRACE environment variable is set.

Related #65

This error type only allows either an error message or an error source as the error message would be stored as a source by converting &str to Box<dyn std::error::Error + 'static>.

This doesn't address #65 point 5.

CosminPerRam commented 1 year ago

Looks really good, although, would we want to have backtracing as default in the library? Maybe have this as a feature? I'm saying this just for regarding performance, but as this would happen once (error has been thrown and thats all), i don't think this would be a problem, and as you said that it may not be an issue because the backtrace is captured only if RUST_BACKTRACE is set.

Douile commented 1 year ago

A macro could look like this:

macro_rules! error_kind_impl {
    ($kind: expr, $name: ident, $from_name: ident) => {
        pub fn $name(source: Option<ErrorSource>) -> GDRichError {
            GDRichError::new($kind, source)
        }
        pub fn $from_name<E: Into<Box<dyn std::error::Error + 'static>>>(source: E) -> GDRichError {
            GDRichError::$name(Some(source.into()))
        }
    }
}

impl GDRichError {
  // ...
  error_kind_impl!(GDError::PacketUnderflow, packet_underflow, packet_underflow_from_into);
  // ...
}

Unfortunately without proc macros both functions names would need to be specified (no way to use macros in place of idents: https://github.com/rust-lang/rust/issues/29599) and each variant needs to be listed.

Douile commented 1 year ago

I cannot believe how much I was over-complicating things. Implementing a single method on the error kind enum is way simpler and works better...

https://github.com/gamedig/rust-gamedig/blob/ebacab398e7a6ac17016372c1c02d5cbc49c7bbf/src/errors.rs#L51-L61

https://github.com/gamedig/rust-gamedig/blob/ebacab398e7a6ac17016372c1c02d5cbc49c7bbf/src/protocols/valve/protocol.rs#L113-L117

Things I would have to do for this PR to be usable (assuming everyone is happy):

CosminPerRam commented 1 year ago

Hey, regarding this point:

Possibly add a separate message field to source field, in case we want to include a message and a source

I think this should be discussed in #65, as this is about giving extra information, and this might be included in the enum by default, we can omit this in this PR.

CosminPerRam commented 1 year ago

Also, please rebase!