Skip to content

Commit

Permalink
Use writeln! over eprintln! to avoid a potential panic (#919)
Browse files Browse the repository at this point in the history
On startup when logging to a file, we use(d) `eprintln!` to leave a note
to the user with the path where logs are emitted. This can cause
processes using dropshot to panic, so this PR replaces `eprintln!` with
`writeln!`.

Fixes #811.
  • Loading branch information
jgallagher authored Feb 26, 2024
1 parent 59f102e commit c37bbb4
Showing 1 changed file with 33 additions and 5 deletions.
38 changes: 33 additions & 5 deletions dropshot/src/logging.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use slog::Level;
use slog::Logger;
use std::fs::OpenOptions;
use std::io::LineWriter;
use std::io::Write;
use std::{io, path::Path};

/// Represents the logging configuration for a server. This is expected to be a
Expand Down Expand Up @@ -101,7 +102,38 @@ impl ConfigLogging {
Path::new(path),
log_name.as_ref().to_string(),
)?;
Ok(async_root_logger(level, drain))
let logger = async_root_logger(level, drain);

// Record a message to the stderr so that a reader who doesn't
// already know how logging is configured knows where the rest
// of the log messages went.
//
// We don't want to use `eprintln!`, because it panics if stderr
// isn't writeable (e.g., if stderr has been redirected to a
// file on a full disk). Our options seem to be:
//
// a) Return an error if we fail to emit this message
// b) Silently swallow errors
// c) If an error occurs, try to relay that fact
//
// (a) doesn't seem great, because it's possible we're able to
// run just fine (and possibly even use the logger we're about
// to create, as we don't know that it will suffer the same fate
// that writing to stderr does). (b) is defensible since this is
// just an informational message. We go with (c) and attempt to
// leave a breadcrumb in the log itself.
if let Err(err) = writeln!(
io::stderr(),
"note: configured to log to \"{path}\"",
) {
slog::warn!(
logger,
"failed to report log path on stderr";
"err" => %err,
);
}

Ok(logger)
}
}
}
Expand Down Expand Up @@ -135,10 +167,6 @@ fn log_drain_for_file(
// Buffer writes to the file around newlines to minimize syscalls.
let file = LineWriter::new(open_options.open(path)?);

// Record a message to the stderr so that a reader who doesn't already know
// how logging is configured knows where the rest of the log messages went.
eprintln!("note: configured to log to \"{}\"", path.display());

// Using leak() here is dubious. However, we really want the logger's name
// to be dynamically generated from the test name. Unfortunately, the
// bunyan interface requires that it be a `&'static str`. The correct
Expand Down

0 comments on commit c37bbb4

Please sign in to comment.