summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorOwen Jacobson <owen@grimoire.ca>2025-11-10 20:12:11 -0500
committerOwen Jacobson <owen@grimoire.ca>2025-11-11 13:55:59 -0500
commitf9ecfa3c342d5932edd5f8d3e75e77482dca472d (patch)
tree0d76c9b7f8580a5891974d348aff364a2077937f
parent91c33501a315abe04aeed54aa27388ce0ad241ce (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.rs1
-rw-r--r--src/error/chain.rs72
-rw-r--r--src/error/mod.rs (renamed from src/error.rs)7
-rw-r--r--src/exit.rs54
-rw-r--r--src/lib.rs1
-rw-r--r--src/main.rs6
6 files changed, 136 insertions, 5 deletions
diff --git a/src/cli.rs b/src/cli.rs
index 154771b..2e66e34 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -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
+ }
+ }
+ }
+}
diff --git a/src/lib.rs b/src/lib.rs
index 38e6bc5..56345d0 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -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()
}