diff options
| author | Owen Jacobson <owen@grimoire.ca> | 2025-11-10 20:12:11 -0500 |
|---|---|---|
| committer | Owen Jacobson <owen@grimoire.ca> | 2025-11-11 13:55:59 -0500 |
| commit | f9ecfa3c342d5932edd5f8d3e75e77482dca472d (patch) | |
| tree | 0d76c9b7f8580a5891974d348aff364a2077937f | |
| parent | 91c33501a315abe04aeed54aa27388ce0ad241ce (diff) | |
Print source chains for errors.
When an error is printed, if that error has a source, print the source. Then repeat that process until there are no more sources.
For an error with no source:
the error message
For an error with a single source:
the error message
caused by:
the source's error message
For an error message with chained sources:
the error message
caused by:
the immediate source's error message
the first transitive source's error message
the second transitive source's error message
…
the ultimate source's error message
That is, sources are printed from the outermost inwards, terminating with the lowest-level source.
This printing occurs in two places: when pilcrow starts, and whenever it processes an internal server error. For internal server errors, the error message is prefixed with the error ID, as before, to allow operators to correlate log messages with responses if they choose to do so:
[E1234] the error message
caused by:
the immediate source's error message
the first transitive source's error message
the second transitive source's error message
…
the ultimate source's error message
Both call sites lock stderr to try to prevent interleaving messages. No promises about how well it works.
I've not made any effort to ensure that multi-line error messages are correctly indented. So far as I know, pilcrow never emits a multi-line error message (except for long messages which are wrapped), so this likely isn't necessary. It's also surprisingly hard to do well, though there is [a crate for it][indenter].
[indenter]: https://docs.rs/indenter/latest/indenter/
There's a widespread convention for unix tools to prefix stderr output with the name of the program. For example, cat on my system prints the following when asked to open a nonexistent file:
cat: nope: No such file or directory
I've opted not to do that for Pilcrow. By my understanding, that convention is intended to help users identify which of several programs emitted a message when several programs share a stderr channel, for example when a program is used as part of a shell pipeline.
Programs that are largely intended to be freestanding do not consistently follow the same convention - Cargo, clang, and gcc all write diagnostics to stderr with no prefix, for example. Pilcrow is, similarly, meant to be run in isolation, either as the single thing a user is running interactively, or because it's being run as a service. In either case, the prefix is not necessary to identify Pilcrow as the source of a message.
I debated making the new `Exit` type crate-private, and having `cli::Args::run` return `impl Termination` to handle the interface between the crate and the outside world. For now, I've opted to instead return a result, so that any ill-advised callers trying to use Pilcrow as a library can at least attempt to understand the structure of its return value; this entails making `Exit` public as well, so that the CLI entry point (`main.rs`) can import it.
This is an enabling change, with little immediate impact on its own. It probably makes some error messages print less than perfectly, but Pilcrow generates errors so rarely in development that it's hard to check. I'll be revising individual errors in later
| -rw-r--r-- | src/cli.rs | 1 | ||||
| -rw-r--r-- | src/error/chain.rs | 72 | ||||
| -rw-r--r-- | src/error/mod.rs (renamed from src/error.rs) | 7 | ||||
| -rw-r--r-- | src/exit.rs | 54 | ||||
| -rw-r--r-- | src/lib.rs | 1 | ||||
| -rw-r--r-- | src/main.rs | 6 |
6 files changed, 136 insertions, 5 deletions
@@ -15,6 +15,7 @@ use sqlx::sqlite::SqlitePool; use tokio::net; use web_push::{IsahcWebPushClient, WebPushClient}; +pub use crate::exit::Exit; use crate::{ app::App, clock, db, routes, diff --git a/src/error/chain.rs b/src/error/chain.rs new file mode 100644 index 0000000..d78fe67 --- /dev/null +++ b/src/error/chain.rs @@ -0,0 +1,72 @@ +use std::{error::Error, io}; + +use super::Id; + +pub fn format<W, E>(out: &mut W, error: E) -> Result<(), io::Error> +where + W: io::Write, + E: Error, +{ + writeln!(out, "{error}")?; + format_sources(out, error)?; + + Ok(()) +} + +pub fn format_with_id<W, E>(out: &mut W, id: &Id, error: E) -> Result<(), io::Error> +where + W: io::Write, + E: Error, +{ + writeln!(out, "[{id}] {error}")?; + format_sources(out, error)?; + + Ok(()) +} + +fn format_sources<W, E>(out: &mut W, error: E) -> Result<(), io::Error> +where + W: io::Write, + E: Error, +{ + let mut sources = Sources::from(&error); + if let Some(source) = sources.next() { + writeln!(out)?; + writeln!(out, "Caused by:")?; + writeln!(out, " {source}")?; + for source in sources { + writeln!(out, " {source}")?; + } + writeln!(out)?; + } + Ok(()) +} + +struct Sources<'e> { + next: Option<&'e dyn Error>, +} + +impl<'e, E> From<&'e E> for Sources<'e> +where + E: Error, +{ + fn from(error: &'e E) -> Self { + Self { + next: error.source(), + } + } +} + +// See also: <https://doc.rust-lang.org/std/error/trait.Error.html#method.sources> However, we only +// want to iterate the sources, and not the error itself. Personally, I find the `skip(1)` +// suggestion untidy, since the error itself is non-optional while the sources are optional. +impl<'a> Iterator for Sources<'a> { + type Item = &'a dyn Error; + + fn next(&mut self) -> Option<Self::Item> { + let source = self.next; + self.next = self.next.and_then(|err| err.source()); + + source + } +} diff --git a/src/error.rs b/src/error/mod.rs index 3c46097..154f98f 100644 --- a/src/error.rs +++ b/src/error/mod.rs @@ -1,10 +1,12 @@ -use std::{error, fmt}; +use std::{error, fmt, io}; use axum::{ http::StatusCode, response::{IntoResponse, Response}, }; +pub mod chain; + // I'm making an effort to avoid `anyhow` here, as that crate is _enormously_ // complex (though very usable). We don't need to be overly careful about // allocations on errors in this app, so this is fine for most "general @@ -38,7 +40,8 @@ impl fmt::Display for Internal { impl IntoResponse for Internal { fn into_response(self) -> Response { let Self(id, error) = &self; - eprintln!("pilcrow: [{id}] {error}"); + chain::format_with_id(&mut io::stderr().lock(), id, error.as_ref()) + .expect("write to stderr"); (StatusCode::INTERNAL_SERVER_ERROR, self.to_string()).into_response() } } diff --git a/src/exit.rs b/src/exit.rs new file mode 100644 index 0000000..3c7b724 --- /dev/null +++ b/src/exit.rs @@ -0,0 +1,54 @@ +use std::{ + error::Error, + io, + process::{ExitCode, Termination}, +}; + +use crate::error::chain; + +/// Formats errors for display before exiting the program. +/// +/// When this is used as the return type of a program's `main`, it can be used to capture any errors +/// during the execution of the program and to display them in a readable format at exit time: +/// +/// ```no_run +/// # use std::{error::Error, io}; +/// fn main() -> pilcrow::cli::Exit<impl Error> { +/// my_complicated_task().into() +/// } +/// +/// fn my_complicated_task() -> Result<(), impl Error> { +/// Err(io::Error::other("stand-in for a real failure")) +/// } +/// ``` +/// +/// If constructed with an `Ok(())`, the resulting `Exit` indicates successful execution, and, when +/// returned from `main`, will cause the program to exit with a successful exit status. If constructed +/// with any `Err(…)` value, the resulting `Exit` indicates unsuccessful execution, and, when +/// returned from `main`, will cause the program to print the error (along with its `source()` +/// chain, if any) before exiting with an unsuccessful exit status. +pub struct Exit<E>(pub Result<(), E>); + +impl<E> From<Result<(), E>> for Exit<E> { + fn from(result: Result<(), E>) -> Self { + Self(result) + } +} + +impl<E> Termination for Exit<E> +where + E: Error, +{ + fn report(self) -> ExitCode { + let Self(result) = self; + match result { + Ok(()) => ExitCode::SUCCESS, + Err(err) => { + // if we can't write the error message to stderr, there's nothing else we can do + // instead, and we're about to exit with a failure anyway. + let _ = chain::format(&mut io::stderr().lock(), &err); + ExitCode::FAILURE + } + } + } +} @@ -12,6 +12,7 @@ mod db; mod empty; mod error; mod event; +pub mod exit; mod expire; mod id; mod invite; diff --git a/src/main.rs b/src/main.rs index f140da1..c7a3602 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,8 +1,8 @@ use clap::Parser; -use pilcrow::cli; +use pilcrow::cli::{self, Exit}; #[tokio::main] -async fn main() -> Result<(), impl std::error::Error> { +async fn main() -> Exit<impl std::error::Error> { let args = cli::Args::parse(); - args.run().await + args.run().await.into() } |
