From 5755de36d6bd2f8ac3cbe1b0a9d93ccf432ed731 Mon Sep 17 00:00:00 2001 From: Ville Nukarinen Date: Fri, 4 Oct 2024 11:33:20 +0300 Subject: [PATCH] Fix logger initialization on readonly filesystems Calling RollingFileAppender::new tries to create the directory for the log files even though logging is turned off by Logger.init('off', null). This change simply postpones the initialization of RollingFileAppender until it is actually needed. This way if we do eg. Logger.init("info", null) it works as expected and does not throw an error on readonly filesystems. Fixes #2387 Signed-off-by: Ville Nukarinen --- logger_core/src/lib.rs | 57 ++++++++++++++++++++++++++++---- logger_core/tests/test_logger.rs | 36 +++++++++++++++++--- 2 files changed, 82 insertions(+), 11 deletions(-) diff --git a/logger_core/src/lib.rs b/logger_core/src/lib.rs index 0bc76a9e91..d1054339a8 100644 --- a/logger_core/src/lib.rs +++ b/logger_core/src/lib.rs @@ -2,9 +2,12 @@ * Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0 */ use once_cell::sync::OnceCell; -use std::sync::RwLock; +use std::{ + path::{Path, PathBuf}, + sync::RwLock, +}; use tracing::{self, event}; -use tracing_appender::rolling::{RollingFileAppender, Rotation}; +use tracing_appender::rolling::{RollingFileAppender, RollingWriter, Rotation}; use tracing_subscriber::{ filter::Filtered, fmt::{ @@ -28,7 +31,7 @@ type InnerLayered = Layered, Registry>; // A reloadable layer of subscriber to a rolling file type FileReload = Handle< Filtered< - Layer, + Layer, LevelFilter, InnerLayered, >, @@ -48,6 +51,47 @@ pub static INITIATE_ONCE: InitiateOnce = InitiateOnce { init_once: OnceCell::new(), }; +const FILE_DIRECTORY: &str = "glide-logs"; + +/// Wraps [RollingFileAppender] to defer initialization until logging is required, +/// allowing [init] to disable file logging on read-only filesystems. +/// This is needed because [RollingFileAppender] tries to create the log directory on initialization. +struct LazyRollingFileAppender { + file_appender: OnceCell, + rotation: Rotation, + directory: PathBuf, + filename_prefix: PathBuf, +} + +impl LazyRollingFileAppender { + fn new( + rotation: Rotation, + directory: impl AsRef, + filename_prefix: impl AsRef, + ) -> LazyRollingFileAppender { + LazyRollingFileAppender { + file_appender: OnceCell::new(), + rotation, + directory: directory.as_ref().to_path_buf(), + filename_prefix: filename_prefix.as_ref().to_path_buf(), + } + } +} + +impl<'a> tracing_subscriber::fmt::writer::MakeWriter<'a> for LazyRollingFileAppender { + type Writer = RollingWriter<'a>; + fn make_writer(&'a self) -> Self::Writer { + let file_appender = self.file_appender.get_or_init(|| { + RollingFileAppender::new( + self.rotation.clone(), + self.directory.clone(), + self.filename_prefix.clone(), + ) + }); + file_appender.make_writer() + } +} + #[derive(Debug)] pub enum Level { Error = 0, @@ -84,9 +128,9 @@ pub fn init(minimal_level: Option, file_name: Option<&str>) -> Level { let (stdout_layer, stdout_reload) = reload::Layer::new(stdout_fmt); - let file_appender = RollingFileAppender::new( + let file_appender = LazyRollingFileAppender::new( Rotation::HOURLY, - "glide-logs", + FILE_DIRECTORY, file_name.unwrap_or("output.log"), ); let file_fmt = tracing_subscriber::fmt::layer() @@ -130,7 +174,8 @@ pub fn init(minimal_level: Option, file_name: Option<&str>) -> Level { }); } Some(file) => { - let file_appender = RollingFileAppender::new(Rotation::HOURLY, "glide-logs", file); + let file_appender = + LazyRollingFileAppender::new(Rotation::HOURLY, FILE_DIRECTORY, file); let _ = reloads .file_reload .write() diff --git a/logger_core/tests/test_logger.rs b/logger_core/tests/test_logger.rs index e38d91e92b..c0b7bbea09 100644 --- a/logger_core/tests/test_logger.rs +++ b/logger_core/tests/test_logger.rs @@ -9,7 +9,10 @@ use test_env_helpers::*; mod tests { use logger_core::{init, log_debug, log_trace}; use rand::{distributions::Alphanumeric, Rng}; - use std::fs::{read_dir, read_to_string, remove_dir_all}; + use std::{ + fs::{read_dir, read_to_string, remove_dir_all}, + path::Path, + }; const FILE_DIRECTORY: &str = "glide-logs"; fn generate_random_string(length: usize) -> String { @@ -38,6 +41,13 @@ mod tests { read_to_string(file.unwrap().path()).unwrap() } + #[test] + fn init_does_not_create_log_directory_when_console_init() { + init(Some(logger_core::Level::Trace), None); + let dir_exists = Path::new(FILE_DIRECTORY).is_dir(); + assert!(!dir_exists); + } + #[test] fn log_to_console_works_after_multiple_inits_diff_log_level() { let identifier = generate_random_string(10); @@ -49,6 +59,16 @@ mod tests { log_trace(identifier, "boo"); } + #[test] + fn log_to_console_does_not_create_log_directory_when_console_init() { + let identifier = generate_random_string(10); + init(Some(logger_core::Level::Trace), None); + // you should see in the console something like '2023-07-07T06:57:54.446236Z TRACE logger_core: e49NaJ5J41 - foo' + log_trace(identifier.clone(), "foo"); + let dir_exists = Path::new(FILE_DIRECTORY).is_dir(); + assert!(!dir_exists); + } + #[test] fn log_to_file_works_after_multiple_inits() { let identifier = generate_random_string(10); @@ -84,14 +104,20 @@ mod tests { } #[test] - fn log_to_file_disabled_when_console_init() { + fn log_to_file_disabled_after_console_init() { let identifier = generate_random_string(10); init(Some(logger_core::Level::Trace), Some(identifier.as_str())); - init(Some(logger_core::Level::Trace), None); - // you should see in the console something like '2023-07-07T06:57:54.446236Z TRACE logger_core: e49NaJ5J41 - foo' log_trace(identifier.clone(), "foo"); + init(Some(logger_core::Level::Trace), None); + log_trace(identifier.clone(), "boo"); let contents = get_file_contents(identifier.as_str()); - assert!(contents.is_empty()); + assert!( + contents.contains(identifier.as_str()), + "Contents: {}", + contents + ); + assert!(contents.contains("foo"), "Contents: {}", contents); + assert!(!contents.contains("boo"), "Contents: {}", contents); } fn clean() -> Result<(), std::io::Error> {