From 42630e42b216e87267e6250e4c7b3b448d63ef8c 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 timezone detection in the presence of multiple threads, because the underlying POSIX function localtime_r is not thread safe. 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 suggesting this fix. There are two improvements that could be made to reduce dependency bloat: - Only enable this on unix systems - Switch to chono entirely. However, this fix is sufficient to avoid the bug and that is the priority. It is possible that the optimizer is smart enough to remove all chrono code except the localtime detection functionality. In that case, switching from time to chrono could increase bloat by pulling in two copies of time formatting code. Tests/benchmarks are needed to determine the best approach. --- CHANGELOG.md | 5 ++++ Cargo.toml | 12 ++++++++ src/lib.rs | 78 ++++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 90 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e144ad3..a1c382f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased](https://github.com/slog-rs/term/compare/v2.8.1...HEAD) - ReleaseDate +### Fixed +* Avoid thread-safety issues with `localtime_r` function by using `chrono` to detect local time. + * Unfortunately this adds another dependency + In the future we could reduce this by switching from time to chrono. + ## 2.9.1 - 2024-02-18 ### Fixed * Switch from `atty` to `is_terminal` diff --git a/Cargo.toml b/Cargo.toml index 9b89506..5e3f775 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,6 +31,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 guaranteed 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 = "0.4" [dev-dependencies] slog-async = "2" diff --git a/src/lib.rs b/src/lib.rs index 03559d0..017ed24 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1091,16 +1091,47 @@ where const TIMESTAMP_FORMAT: &[time::format_description::FormatItem] = time::macros::format_description!("[month repr:short] [day] [hour repr:24]:[minute]:[second].[subsecond digits:3]"); +/// Convert from [`chrono::DateTime`] into [`time::OffsetDateTime`] +fn local_timestamp_from_chrono( + local_time: chrono::DateTime, +) -> result::Result { + use chrono::Utc; + 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.to_rfc3339(), local_time.to_rfc3339()); + } else { + assert_ne!(utc_time.to_rfc3339(), local_time.to_rfc3339()); + } + } + let utc_time: time::OffsetDateTime = + time::OffsetDateTime::from_unix_timestamp(utc_time.timestamp()) + .map_err(TimestampError::LocalTimeConversion)? + + time::Duration::nanoseconds(i64::from( + utc_time.timestamp_subsec_nanos(), + )); + Ok(utc_time.to_offset( + time::UtcOffset::from_whole_seconds(offset.local_minus_utc()) + .map_err(TimestampError::LocalTimeConversion)?, + )) +} + /// 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 dependency, PR #44 and PR #48. 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())?; write!( io, "{}", now.format(TIMESTAMP_FORMAT) - .map_err(convert_time_fmt_error)? + .map_err(TimestampError::Format)? ) } @@ -1113,11 +1144,48 @@ 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), + } + } } // }}}