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 valkey-io#2387

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<reload::Layer<InnerFiltered, Registry>, Registry>;
 // A reloadable layer of subscriber to a rolling file
 type FileReload = Handle<
     Filtered<
-        Layer<InnerLayered, DefaultFields, Format, RollingFileAppender>,
+        Layer<InnerLayered, DefaultFields, Format, LazyRollingFileAppender>,
         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<RollingFileAppender>,
+    rotation: Rotation,
+    directory: PathBuf,
+    filename_prefix: PathBuf,
+}
+
+impl LazyRollingFileAppender {
+    fn new(
+        rotation: Rotation,
+        directory: impl AsRef<Path>,
+        filename_prefix: impl AsRef<Path>,
+    ) -> 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<Level>, 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<Level>, 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()

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> {