mullvad / windows-service-rs

Windows services in Rust
Apache License 2.0
525 stars 85 forks source link

feat: write detailed IO error info in winapi call #128

Closed ArcticLampyrid closed 7 months ago

ArcticLampyrid commented 7 months ago

This change is Reviewable

pronebird commented 7 months ago

Yes but why? The Error type exposes source() so either fetch it or use a function/crate to unroll the error chain.

For example if you use anyhow you can unroll the error chain by using a {:#} format. This would also prevent duplicate messages from appearing at each level, i.e:

Error: IO error in winapi call

Caused by:
  No such file or directory (os error 2)

as opposed to:

Error: IO error in winapi call: No such file or directory (os error 2)

Caused by:
  No such file or directory (os error 2)
ArcticLampyrid commented 7 months ago

I'm using error!("ABC: {}", xxx) to log errors, which seems not to record anything about source()

Maybe I need to change my way of logging?

ArcticLampyrid commented 7 months ago

For example if you use anyhow you can unroll the error chain by using a {:#} format. This would also prevent duplicate messages from appearing at each level.

Curious whether this is a prescriptive error handling method.

I'm using thiserror to wrap every error. And then there is no such magic for thiserror to handle {:#} format.

pronebird commented 7 months ago

A bit while ago @faern wondered the same thing

I believe the correct thing to do is to leave the details of source error to the source error itself, but maybe others have a different view.

I have been carrying this small helper to print error chains for years and I hope one day std::error::Error gains a method to do the same. Especially given that we already have source() implemented as a part of std::error::Error. But I guess different people have different preferences in terms of output, so maybe that's what impedes the introduction of such helper into std. Personally I dislike the output of anyhow and prefer the helper above.

ArcticLampyrid commented 7 months ago

Make sense.