From 4465f02985c49d7b5513eeef626b91789e207e34 Mon Sep 17 00:00:00 2001 From: Techcable Date: Sun, 18 Feb 2024 16:13:34 -0700 Subject: [PATCH] Use chrono to detect local timezone thread-safely As discussed in PR #44, the 'time' crate fails timezine detection in the prsesense of multiple threads, because the underlying POSIX function localtime_r is not thread safe (at least not on Linux). In order to avoid these thread-safety issues, we use the chrono crate. It avoids system libraries and reimplements timezone detection from scratch. (Thanks to @yaozongyou for pointing this out) TODO: Maybe we should switch to chono entirely. It seems to be a fairly complete replacement. What are the advantages & disadvantages? --- Cargo.toml | 27 ++++++++++++++++ src/lib.rs | 81 ++++++++++++++++++++++++++++++++++++++++++++---- src/timestamp.rs | 22 +++++++++++++ 3 files changed, 124 insertions(+), 6 deletions(-) create mode 100644 src/timestamp.rs diff --git a/Cargo.toml b/Cargo.toml index 5d9c9a9..83a7e7e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,7 +20,22 @@ edition = "2018" rust-version = "1.63" [features] +# Enabled for compatibility reasons +default = ["localtime"] nested-values = ["erased-serde", "serde", "serde_json", "slog/nested-values"] +# Automatically detect and use the local timezone for timestamps. +# +# This is enabled by default for compatibility reasons, +# but can be explicitly disabled to reduce dependencies. +# +# This option only affects the default behavior +# and enables support for automatic selection. +# +# Either way an explicit timezone suffix is added for clarity +# UTC: 21:43+00 +# MST: 14:43-07 +localtime = ["chrono"] + [dependencies] slog = "2" @@ -31,6 +46,18 @@ term = "0.7" erased-serde = {version = "0.3", optional = true } serde = {version = "1.0", optional = true } serde_json = {version = "1.0", optional = true } +# We use chrono is to determine the local timezone offset. +# This is because time uses POSIX localtime_r, +# which is not guarenteed to be thread-safe +# +# However, chrono _is_ threadsafe. It works around this issue by +# using a custom reimplementation of the timezone detection logic. +# +# Therefore, whenever the feature 'localtime' is requested, +# we add a dependency on chrono to detect the timezone. +# +# See PR #44 for the original discussion of this issue. +chrono = { version = "0.4", optional = true }d [dev-dependencies] slog-async = "2" diff --git a/src/lib.rs b/src/lib.rs index 3603692..61542f0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -356,6 +356,7 @@ where } /// Use the local time zone for the timestamp (default) + #[cfg(feature = "localtime")] pub fn use_local_timestamp(mut self) -> Self { self.fn_timestamp = Box::new(timestamp_local); self @@ -1089,18 +1090,53 @@ where { } -const TIMESTAMP_FORMAT: &[time::format_description::FormatItem] = time::macros::format_description!("[month repr:short] [day] [hour repr:24]:[minute]:[second].[subsecond digits:3]"); +const TIMESTAMP_FORMAT: &[time::format_description::FormatItem] = time::macros::format_description!( + concat!( + "[month repr:short] [day]", + "[hour repr:24]:[minute]:[second].[subsecond digits:3]", + // Include timezone by default, to avoid ambiguity + "[offset_hour sign:mandatory]" + ) +); + +/// Convert from [`chrono::DateTime`] into [`time::OffsetDateTime`] +#[cfg(feature = "localtime")] +fn local_timestamp_from_chrono( + local: chrono::DateTime +) -> result::Result { + use chrono::{Local, Utc}; + let local_time: chrono::DateTime = Local::now(); + let offset: chrono::FixedOffset = local_time.fixed_offset().timezone(); + let utc_time: chrono::DateTime = local_time.to_utc(); + #[cfg(test)] { + if offset.local_minus_utc() == 0 { + assert_eq!(utc_time.date_naive(), local_time.date_naive()); + } else { + assert_ne!(utc_time.date_naive(), local_time.date_naive()); + } + } + let utc_time: time::OffsetDateTime = time::OffsetDateTime::from_unix_timestamp(utc_time.timestamp()) + .map_err(LocalTimestampError::UnixTimestamp) + + time::Duration::nanoseconds(i64::from(utc_time.timestamp_subsec_nanos())); + Ok(utc_time.to_offset(time::UtcOffset::from_whole_seconds(offset.local_minus_utc())?)) +} /// Default local timezone timestamp function /// /// The exact format used, is still subject to change. +/// +/// # Implementation Note +/// This requires `chrono` to detect the localtime in a thread-safe manner. +/// See the comment on the feature flag and PR #44 +#[cfg(feature = "localtime")] pub fn timestamp_local(io: &mut dyn io::Write) -> io::Result<()> { - let now: time::OffsetDateTime = std::time::SystemTime::now().into(); + let now = local_timestamp_from_chrono(chrono::Local::now()) + .map_err(TimestampError::LocalConversion)?; write!( io, "{}", now.format(TIMESTAMP_FORMAT) - .map_err(convert_time_fmt_error)? + .map_err(TimestampError::Format)? ) } @@ -1113,11 +1149,44 @@ pub fn timestamp_utc(io: &mut dyn io::Write) -> io::Result<()> { io, "{}", now.format(TIMESTAMP_FORMAT) - .map_err(convert_time_fmt_error)? + .map_err(TimestampError::Format)? ) } -fn convert_time_fmt_error(cause: time::error::Format) -> io::Error { - io::Error::new(io::ErrorKind::Other, cause) + +/// An internal error that occurs with timestamps +#[derive(Debug)] +#[non_exhaustive] +enum TimestampError { + /// An error formatting the timestamp + Format(time::error::Format), + /// An error that occurred while + /// converting from `chrono::DateTime` to `time::OffsetDateTime` + LocalTimeConversion(time::error::ComponentRange) +} +/* + * We could use thiserror here, + * but writing it by hand is almost as good and eliminates a dependency + */ +impl fmt::Display for TimestampError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + TimestampError::Format(cause) => write!(f, "Failed to format timestamp: {cause}"), + TimestampError::LocalTimeConversion(cause) => write!(f, "Failed to convert local time: {cause}") + } + } +} +impl From for io::Error { + fn from(cause: TimestampError) -> Self { + io::Error::new(io::ErrorKind::Other, cause) + } +} +impl std::error::Error for TimestampError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + TimestampError::Format(cause) => Some(cause), + TimestampError::LocalTimeConversion(cause) => Some(cause), + } + } } // }}} diff --git a/src/timestamp.rs b/src/timestamp.rs new file mode 100644 index 0000000..24f2e65 --- /dev/null +++ b/src/timestamp.rs @@ -0,0 +1,22 @@ +//! Timestamp logic for slog-term + +/// A threadsafe timestamp formatter +/// +/// To satify `slog-rs` thread and unwind safety requirements, the +/// bounds expressed by this trait need to satisfied for a function +/// to be used in timestamp formatting. +pub trait TimestampWriter: Sealed + Send + Sync + UnwindSafe + RefUnwindSafe + 'static { + fn write_timestamp(&self, writer: &mut dyn io::Write) -> io::Result<()>; +} + + +impl ThreadSafeTimestampFn for F +where + F: Fn(&mut dyn io::Write) -> io::Result<()> + Send + Sync, + F: UnwindSafe + RefUnwindSafe + 'static, + F: ?Sized, +{} + +mod sealed { + pub trait Sealed {} +} \ No newline at end of file