Logging #26

Open
leon wants to merge 33 commits from Logging into main
3 changed files with 212 additions and 201 deletions
Showing only changes of commit 9110c16256 - Show all commits

View File

@ -81,14 +81,12 @@ impl Config {
/// Getter for [`Self::log_path`].
#[must_use]
pub fn log_path(&self) -> Option<&Path>
{
pub fn log_path(&self) -> Option<&Path> {
self.log_path.as_deref()
}
/// Setter for [`Self::log_path`].
pub fn set_log_path(&mut self, log_path: impl Into<Option<PathBuf>>)
{
pub fn set_log_path(&mut self, log_path: impl Into<Option<PathBuf>>) {
self.log_path = log_path.into();
}
}

View File

@ -9,8 +9,8 @@ use std::fmt::Display;
use std::fs::OpenOptions;
use std::io::Write;
use config::LogVerbosity;
use config::Config;
use config::LogVerbosity;
use chrono::Utc;
@ -30,7 +30,6 @@ use LogMessageType::GenericWarn;
* or if another error occurs, such as a full disk.
*/
pub fn log_message(msg: &LogMessage, conf: &Config, file: &str, line: u32, column: u32) {
let Some(log_line) = log_to_str(msg, conf, file, line, column) else { return };
// May panic if file cannot be opened or written to.
@ -42,11 +41,7 @@ pub fn log_message(msg: &LogMessage, conf: &Config, file: &str, line: u32, colum
.append(true)
.create(true)
.open(path)
.unwrap_or_else(|_| {
panic!(
"Could not open log file: {path:#?}"
)
});
.unwrap_or_else(|_| panic!("Could not open log file: {path:#?}"));
writeln!(file, "{log_line}")
.unwrap_or_else(|_| panic!("Could not write log to file: {path:#?}"));
},
@ -68,8 +63,13 @@ pub fn log_message(msg: &LogMessage, conf: &Config, file: &str, line: u32, colum
* or if another error occurs, such as a full disk.
*/
#[must_use]
pub fn log_to_str(msg: &LogMessage, conf: &Config, file: &str, line: u32, column: u32) -> Option<String>
{
pub fn log_to_str(
msg: &LogMessage,
conf: &Config,
file: &str,
line: u32,
column: u32,
) -> Option<String> {
if conf.log_verbosity() < &msg.1 {
return None;
}
@ -95,12 +95,12 @@ pub fn log_to_str(msg: &LogMessage, conf: &Config, file: &str, line: u32, column
#[macro_export]
macro_rules! log {
($msg:expr) => {
let conf = config::CONFIG.read().unwrap_or_else(|_| {
panic!("Failed aqcuire read lock on config!")
});
let conf = config::CONFIG
.read()
.unwrap_or_else(|_| panic!("Failed aqcuire read lock on config!"));
log_message($msg, &*conf, file!(), line!(), column!());
drop(conf);
Outdated
Review

logging/src/lib.rs:102 will panic if any code requests write-lock on config::CONFIG and then panics. Consider using Arc to ensure immutability and accessibility to the configuration from any thread at any time

Fair, although getting a mut ref on an Arc would require some while-looping everytime.

The same question about read-only config applies here:
https://git.libre.moe/KomuSolutions/WANessa/pulls/26/files#issuecomment-1519

When this function is stable, it might be used to clear the poison:
https://doc.rust-lang.org/std/sync/struct.RwLock.html#method.clear_poison

> logging/src/lib.rs:102 will panic if any code requests write-lock on config::CONFIG and then panics. Consider using Arc to ensure immutability and accessibility to the configuration from any thread at any time Fair, although getting a mut ref on an Arc would require some while-looping everytime. The same question about read-only config applies here: https://git.libre.moe/KomuSolutions/WANessa/pulls/26/files#issuecomment-1519 When this function is stable, it might be used to clear the poison: https://doc.rust-lang.org/std/sync/struct.RwLock.html#method.clear_poison
Outdated
Review
@hendrik

Assuming the config is read-only, we could also use an Arc without while-looping everytime by using the Arc::clone() function. Also using Arcs would defeat the whole problem of poisoning due to no code being able to request a write-lock.

@leon

Assuming the config is read-only, we could also use an Arc without while-looping everytime by using the Arc::clone() function. Also using Arcs would defeat the whole problem of poisoning due to no code being able to request a write-lock. @leon
Outdated
Review

While not a read lock, the Arc Type has this, which seems to be at odds with the whole immutability thing.
https://doc.rust-lang.org/std/sync/struct.Arc.html#method.get_mut
The docs don't seem to define what happens after you get the &mut and then panic before dropping the reference, which seems sketchy to me

To replace the compile-time default config with the runtime sysadmin config, we need to get a *mut reference for mem::swap to work. Otherwise, we can only have the default values, which defeats the point of a config.

While not a read lock, the Arc Type has this, which seems to be at odds with the whole immutability thing. https://doc.rust-lang.org/std/sync/struct.Arc.html#method.get_mut The docs don't seem to define what happens after you get the &mut and then panic before dropping the reference, which seems sketchy to me To replace the compile-time default config with the runtime sysadmin config, we need to get a *mut reference for [`mem::swap`](https://doc.rust-lang.org/std/mem/fn.swap.html) to work. Otherwise, we can only have the default values, which defeats the point of a config.
Outdated
Review

Or alternatively, we could use this crate I once used in a test:
https://docs.rs/once_cell/latest/once_cell/sync/struct.Lazy.html

It would also make DEFAULT more sensible, since it can now use strings and other types of variable length.

Or alternatively, we could use this crate I once used in a test: https://docs.rs/once_cell/latest/once_cell/sync/struct.Lazy.html It would also make DEFAULT more sensible, since it can now use strings and other types of variable length.
Outdated
Review

See a787dd93e5

TODO: Poison checking.

See a787dd93e5 TODO: Poison checking.
}
};
}
/**

View File

@ -3,7 +3,13 @@
* This test suite uses uuid to easily avoid race conditions when writing to the same log file.
*/
use std::{collections::HashSet, env, fs::{create_dir_all, read_to_string}, mem::take, path::PathBuf};
use std::{
collections::HashSet,
env,
fs::{create_dir_all, read_to_string},
mem::take,
path::PathBuf,
};
use futures::{executor::block_on, future::join_all};
use once_cell::sync::Lazy;
@ -14,25 +20,24 @@ use super::*;
const CONCURRENT_MESSAGE_COUNT: usize = 99999;
static LOG_DIR: Lazy<String> = Lazy::new(||
if cfg!(unix)
{
static LOG_DIR: Lazy<String> = Lazy::new(|| {
if cfg!(unix) {
String::from("/tmp/WANessa/unit-tests/logging")
}
else if cfg !(windows) {
let tmp_path = env::var("TMP").unwrap_or_else(|_| env::var("TEMP").expect("Windows should have both TMP and TEMP, but you have neither, what did you do?"));
} else if cfg!(windows) {
let tmp_path = env::var("TMP").unwrap_or_else(|_| {
env::var("TEMP").expect(
"Windows should have both TMP and TEMP, but you have neither, what did you do?",
)
});
format!("{tmp_path}/WANessa/unit-tests/logging")
} else {
String::new()
}
else { String::new() }
);
});
/// Tests if [`log_message`] to file correctly.
#[test]
pub fn log_msg_file()
{
pub fn log_msg_file() {
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let message = LogMessageType::GenericWarn(String::from("Test Log")).log_message();
@ -52,8 +57,7 @@ pub fn log_msg_file()
/// Tests if [`log_message`] does not modify output from [`log_to_str`], when logging to file.
#[test]
pub fn log_str()
{
pub fn log_str() {
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let message = LogMessageType::GenericWarn(String::from("Test Log")).log_message();
@ -63,7 +67,9 @@ pub fn log_str()
create_dir_all(PathBuf::from(LOG_DIR.as_str()))
.unwrap_or_else(|_| panic!("Could not create directory: {}", *LOG_DIR));
let log_line = log_to_str(&message, &config, file!(), line!(), column!()).expect("There should be a log line.") + "\n";
let log_line = log_to_str(&message, &config, file!(), line!(), column!())
.expect("There should be a log line.")
+ "\n";
log_message(&message, &config, file!(), line!(), column!());
@ -75,8 +81,7 @@ pub fn log_str()
/// Tests if no messages are unintentionally filtered due to their verboisity.
#[test]
pub fn verbosity_no_filter()
{
pub fn verbosity_no_filter() {
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let messages = vec![
@ -92,10 +97,11 @@ pub fn verbosity_no_filter()
create_dir_all(PathBuf::from(LOG_DIR.as_str()))
.unwrap_or_else(|_| panic!("Could not create directory: {}", *LOG_DIR));
let log_line = log_to_str(&messages[0], &config, file!(), line!(), column!()).expect("There should be a log line.") + "\n";
let log_line = log_to_str(&messages[0], &config, file!(), line!(), column!())
.expect("There should be a log line.")
+ "\n";
for msg in messages
{
for msg in messages {
log_message(&msg, &config, file!(), line!(), column!());
}
@ -107,8 +113,7 @@ pub fn verbosity_no_filter()
/// Tests if messages are properly filtered according to their verbosity.
#[test]
pub fn verbosity_filter()
{
pub fn verbosity_filter() {
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let messages = vec![
@ -123,11 +128,14 @@ pub fn verbosity_filter()
create_dir_all(PathBuf::from(LOG_DIR.as_str()))
.unwrap_or_else(|_| panic!("Could not create directory: {}", *LOG_DIR));
let mut log_line = log_to_str(&messages[0], &config, file!(), line!(), column!()).expect("There should be a log line.") + "\n";
log_line += &(log_to_str(&messages[1], &config, file!(), line!(), column!()).expect("There should be a log line.") + "\n");
let mut log_line = log_to_str(&messages[0], &config, file!(), line!(), column!())
.expect("There should be a log line.")
+ "\n";
log_line += &(log_to_str(&messages[1], &config, file!(), line!(), column!())
.expect("There should be a log line.")
+ "\n");
for msg in messages
{
for msg in messages {
log_message(&msg, &config, file!(), line!(), column!());
}
@ -137,7 +145,6 @@ pub fn verbosity_filter()
assert_eq!(log_file, log_line);
}
/**
* All testing concurrency in a controlled manner.
*
@ -145,8 +152,7 @@ pub fn verbosity_filter()
* it might read the result from the changed config.
*/
#[test]
pub fn concurrency_tests()
{
pub fn concurrency_tests() {
log_macro_file();
log_concurrently_any_order();
log_concurrently_correct_order();
@ -156,12 +162,13 @@ pub fn concurrency_tests()
* Tests if log macro logs to file correctly.
* [`config::CONFIG`]
*/
fn log_macro_file()
{
fn log_macro_file() {
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let message = LogMessageType::GenericWarn(String::from("Test Log")).log_message();
let mut config = config::CONFIG.write().expect("Could not acquire write lock on config!");
let mut config = config::CONFIG
.write()
.expect("Could not acquire write lock on config!");
take(&mut *config);
config.set_log_path(PathBuf::from(log_path));
drop(config);
@ -180,38 +187,41 @@ fn log_macro_file()
/**
* Tests concurrent logging. Log lines may be in any order.
*/
fn log_concurrently_any_order()
{
fn log_concurrently_any_order() {
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let mut config = config::CONFIG.write().expect("Could not acquire write lock on config!");
let mut config = config::CONFIG
.write()
.expect("Could not acquire write lock on config!");
take(&mut *config);
let mut messages = Vec::with_capacity(CONCURRENT_MESSAGE_COUNT);
config.set_log_path(PathBuf::from(log_path));
drop(config);
for i in 0..CONCURRENT_MESSAGE_COUNT
{
for i in 0..CONCURRENT_MESSAGE_COUNT {
let msg = i.to_string();
messages.push(
async {
messages.push(async {
log!(&LogMessageType::GenericWarn(msg).log_message());
}
);
});
}
block_on(join_all(messages));
let mut num_set = HashSet::with_capacity(CONCURRENT_MESSAGE_COUNT);
for i in 0..CONCURRENT_MESSAGE_COUNT
{
for i in 0..CONCURRENT_MESSAGE_COUNT {
num_set.insert(i);
}
for line in read_to_string(PathBuf::from(log_path)).unwrap_or_else(|_| panic!("Could not read file: {log_path}")).lines()
for line in read_to_string(PathBuf::from(log_path))
.unwrap_or_else(|_| panic!("Could not read file: {log_path}"))
.lines()
{
let num_str = line.split_whitespace().last().unwrap_or_else(|| panic!("Could not get message number from line: {line}"));
let num = usize::from_str_radix(num_str, 10).unwrap_or_else(|_| panic!("Could not parse number: {num_str}"));
let num_str = line
.split_whitespace()
.last()
.unwrap_or_else(|| panic!("Could not get message number from line: {line}"));
let num = usize::from_str_radix(num_str, 10)
.unwrap_or_else(|_| panic!("Could not parse number: {num_str}"));
assert!(num_set.remove(&num));
}
@ -221,42 +231,45 @@ fn log_concurrently_any_order()
/**
* Tests concurrent logging. Log lines must be in order.
*/
fn log_concurrently_correct_order()
{
fn log_concurrently_correct_order() {
let log_path = &format!("{}/{}", *LOG_DIR, Uuid::new_v4());
println!("Log Path: {log_path}");
let mut config = config::CONFIG.write().expect("Could not acquire write lock on config!");
let mut config = config::CONFIG
.write()
.expect("Could not acquire write lock on config!");
take(&mut *config);
let mut messages = Vec::with_capacity(CONCURRENT_MESSAGE_COUNT);
config.set_log_path(PathBuf::from(log_path));
drop(config);
for i in 0..CONCURRENT_MESSAGE_COUNT
{
for i in 0..CONCURRENT_MESSAGE_COUNT {
let msg = i.to_string();
messages.push(
async {
messages.push(async {
log!(&LogMessageType::GenericWarn(msg).log_message());
}
);
});
}
block_on(join_all(messages));
let mut num_set = HashSet::with_capacity(CONCURRENT_MESSAGE_COUNT);
for i in 0..CONCURRENT_MESSAGE_COUNT
{
for i in 0..CONCURRENT_MESSAGE_COUNT {
num_set.insert(i);
}
for (i, line) in read_to_string(PathBuf::from(log_path)).unwrap_or_else(|_| panic!("Could not read file: {log_path}")).lines().enumerate()
for (i, line) in read_to_string(PathBuf::from(log_path))
.unwrap_or_else(|_| panic!("Could not read file: {log_path}"))
.lines()
.enumerate()
{
let num_str = line.split_whitespace().last().unwrap_or_else(|| panic!("Could not get message number from line: {line}"));
let num = usize::from_str_radix(num_str, 10).unwrap_or_else(|_| panic!("Could not parse number: {num_str}"));
let num_str = line
.split_whitespace()
.last()
.unwrap_or_else(|| panic!("Could not get message number from line: {line}"));
let num = usize::from_str_radix(num_str, 10)
.unwrap_or_else(|_| panic!("Could not parse number: {num_str}"));
assert!(num_set.remove(&num));
assert_eq!(i, num);
}
assert_eq!(num_set.len(), 0);
}