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.

Author: Techcable

CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 <!-- next-url -->
 ## [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`

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 = { version = "0.4", optional = true }
 
 [dev-dependencies]
 slog-async = "2"

src/lib.rs

@@ -1091,16 +1091,48 @@ 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: chrono::DateTime<chrono::Local>,
+) -> result::Result<time::OffsetDateTime, TimestampError> {
+    use chrono::{Local, Utc};
+    let local_time: chrono::DateTime<Local> = Local::now();
+    let offset: chrono::FixedOffset = local_time.fixed_offset().timezone();
+    let utc_time: chrono::DateTime<Utc> = 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 +1145,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<Local>` 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<TimestampError> 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),
+        }
+    }
 }
 
 // }}}

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<F> 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 {}
+}