From 9ffdf460353252d131b24fcbeac9abeb5bb1a8aa 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 --- logger_core/src/lib.rs | 49 ++++++++++++++++++++++++++++---- logger_core/tests/test_logger.rs | 36 +++++++++++++++++++---- 2 files changed, 75 insertions(+), 10 deletions(-) diff --git a/logger_core/src/lib.rs b/logger_core/src/lib.rs index 0bc76a9e91..6145b7d53f 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,42 @@ pub static INITIATE_ONCE: InitiateOnce = InitiateOnce { init_once: OnceCell::new(), }; +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,7 +123,7 @@ 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_name.unwrap_or("output.log"), @@ -130,7 +169,7 @@ 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, "glide-logs", file); let _ = reloads .file_reload .write() diff --git a/logger_core/tests/test_logger.rs b/logger_core/tests/test_logger.rs index e38d91e92b..911abb57f0 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 == false); + } + #[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 == false); + } + #[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> {