From 31ee3feb57459854f0d5c70909d8c5c1d278cb7c Mon Sep 17 00:00:00 2001 From: iequidoo Date: Thu, 14 Dec 2023 20:42:25 -0300 Subject: [PATCH] fix: Use SystemTime instead of Instant everywhere If a time value doesn't need to be sent to another host, saved to the db or otherwise used across program restarts, a monotonically nondecreasing clock (`Instant`) should be used. But as `Instant` may use `libc::clock_gettime(CLOCK_MONOTONIC)`, e.g. on Android, and does not advance while being in deep sleep mode, get rid of `Instant` in favor of using `SystemTime`, but add `tools::Time` as an alias for it with the appropriate comment so that it's clear why `Instant` isn't used in those places and to protect from unwanted usages of `Instant` in the future. Also this can help to switch to another clock impl if we find any. --- src/constants.rs | 2 +- src/context.rs | 31 +++++++++++++++++-------------- src/imap/idle.rs | 11 ++++------- src/imap/scan_folders.rs | 7 ++++--- src/key.rs | 6 +++--- src/quota.rs | 8 ++++---- src/scheduler.rs | 9 +++++---- src/smtp.rs | 13 +++++-------- src/smtp/send.rs | 3 ++- src/tools.rs | 11 +++++++++++ 10 files changed, 56 insertions(+), 45 deletions(-) diff --git a/src/constants.rs b/src/constants.rs index e5bd61286..a8747b9c6 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -217,7 +217,7 @@ pub(crate) const DC_FOLDERS_CONFIGURED_VERSION: i32 = 4; pub(crate) const DEFAULT_MAX_SMTP_RCPT_TO: usize = 50; /// How far the last quota check needs to be in the past to be checked by the background function (in seconds). -pub(crate) const DC_BACKGROUND_FETCH_QUOTA_CHECK_RATELIMIT: i64 = 12 * 60 * 60; // 12 hours +pub(crate) const DC_BACKGROUND_FETCH_QUOTA_CHECK_RATELIMIT: u64 = 12 * 60 * 60; // 12 hours #[cfg(test)] mod tests { diff --git a/src/context.rs b/src/context.rs index bdd11a548..3f3365f51 100644 --- a/src/context.rs +++ b/src/context.rs @@ -6,7 +6,7 @@ use std::ops::Deref; use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; -use std::time::{Duration, Instant, SystemTime}; +use std::time::Duration; use anyhow::{bail, ensure, Context as _, Result}; use async_channel::{self as channel, Receiver, Sender}; @@ -33,7 +33,7 @@ use crate::scheduler::{convert_folder_meaning, SchedulerState}; use crate::sql::Sql; use crate::stock_str::StockStrings; use crate::timesmearing::SmearedTimestamp; -use crate::tools::{create_id, duration_to_str, time}; +use crate::tools::{self, create_id, duration_to_str, time, time_elapsed}; /// Builder for the [`Context`]. /// @@ -233,7 +233,7 @@ pub struct InnerContext { /// IMAP METADATA. pub(crate) metadata: RwLock>, - pub(crate) last_full_folder_scan: Mutex>, + pub(crate) last_full_folder_scan: Mutex>, /// ID for this `Context` in the current process. /// @@ -241,7 +241,7 @@ pub struct InnerContext { /// be identified by this ID. pub(crate) id: u32, - creation_time: SystemTime, + creation_time: tools::Time, /// The text of the last error logged and emitted as an event. /// If the ui wants to display an error after a failure, @@ -262,7 +262,7 @@ enum RunningState { Running { cancel_sender: Sender<()> }, /// Cancel signal has been sent, waiting for ongoing process to be freed. - ShallStop { request: Instant }, + ShallStop { request: tools::Time }, /// There is no ongoing process, a new one can be allocated. Stopped, @@ -394,7 +394,7 @@ impl Context { new_msgs_notify, server_id: RwLock::new(None), metadata: RwLock::new(None), - creation_time: std::time::SystemTime::now(), + creation_time: tools::Time::now(), last_full_folder_scan: Mutex::new(None), last_error: std::sync::RwLock::new("".to_string()), debug_logging: std::sync::RwLock::new(None), @@ -454,7 +454,7 @@ impl Context { } let address = self.get_primary_self_addr().await?; - let time_start = std::time::SystemTime::now(); + let time_start = tools::Time::now(); info!(self, "background_fetch started fetching {address}"); let _pause_guard = self.scheduler.pause(self.clone()).await?; @@ -476,7 +476,10 @@ impl Context { let quota = self.quota.read().await; quota .as_ref() - .filter(|quota| quota.modified + DC_BACKGROUND_FETCH_QUOTA_CHECK_RATELIMIT > time()) + .filter(|quota| { + time_elapsed("a.modified) + > Duration::from_secs(DC_BACKGROUND_FETCH_QUOTA_CHECK_RATELIMIT) + }) .is_none() }; @@ -489,7 +492,7 @@ impl Context { info!( self, "background_fetch done for {address} took {:?}", - time_start.elapsed().unwrap_or_default() + time_elapsed(&time_start), ); Ok(()) @@ -591,7 +594,7 @@ impl Context { pub(crate) async fn free_ongoing(&self) { let mut s = self.running_state.write().await; if let RunningState::ShallStop { request } = *s { - info!(self, "Ongoing stopped in {:?}", request.elapsed()); + info!(self, "Ongoing stopped in {:?}", time_elapsed(&request)); } *s = RunningState::Stopped; } @@ -606,7 +609,7 @@ impl Context { } info!(self, "Signaling the ongoing process to stop ASAP.",); *s = RunningState::ShallStop { - request: Instant::now(), + request: tools::Time::now(), }; } RunningState::ShallStop { .. } | RunningState::Stopped => { @@ -867,8 +870,8 @@ impl Context { .to_string(), ); - let elapsed = self.creation_time.elapsed(); - res.insert("uptime", duration_to_str(elapsed.unwrap_or_default())); + let elapsed = time_elapsed(&self.creation_time); + res.insert("uptime", duration_to_str(elapsed)); Ok(res) } @@ -1214,7 +1217,7 @@ pub fn get_version_str() -> &'static str { #[cfg(test)] mod tests { - use std::time::Duration; + use std::time::{Duration, SystemTime}; use anyhow::Context as _; use strum::IntoEnumIterator; diff --git a/src/imap/idle.rs b/src/imap/idle.rs index 6fcb768e2..4980bbc2c 100644 --- a/src/imap/idle.rs +++ b/src/imap/idle.rs @@ -1,4 +1,4 @@ -use std::time::{Duration, SystemTime}; +use std::time::Duration; use anyhow::{bail, Context as _, Result}; use async_channel::Receiver; @@ -11,6 +11,7 @@ use crate::config::Config; use crate::context::Context; use crate::imap::{client::IMAP_TIMEOUT, FolderMeaning}; use crate::log::LogExt; +use crate::tools::{self, time_elapsed}; /// Timeout after which IDLE is finished /// if there are no responses from the server. @@ -106,7 +107,7 @@ impl Imap { // Idle using polling. This is also needed if we're not yet configured - // in this case, we're waiting for a configure job (and an interrupt). - let fake_idle_start_time = SystemTime::now(); + let fake_idle_start_time = tools::Time::now(); // Do not poll, just wait for an interrupt when no folder is passed in. let watch_folder = if let Some(watch_folder) = watch_folder { @@ -196,11 +197,7 @@ impl Imap { info!( context, "IMAP-fake-IDLE done after {:.4}s", - SystemTime::now() - .duration_since(fake_idle_start_time) - .unwrap_or_default() - .as_millis() as f64 - / 1000., + time_elapsed(&fake_idle_start_time).as_millis() as f64 / 1000., ); } } diff --git a/src/imap/scan_folders.rs b/src/imap/scan_folders.rs index 257d1a483..15aac412b 100644 --- a/src/imap/scan_folders.rs +++ b/src/imap/scan_folders.rs @@ -1,4 +1,4 @@ -use std::{collections::BTreeMap, time::Instant}; +use std::collections::BTreeMap; use anyhow::{Context as _, Result}; use futures::TryStreamExt; @@ -7,6 +7,7 @@ use super::{get_folder_meaning_by_attrs, get_folder_meaning_by_name}; use crate::config::Config; use crate::imap::Imap; use crate::log::LogExt; +use crate::tools::{self, time_elapsed}; use crate::{context::Context, imap::FolderMeaning}; impl Imap { @@ -15,7 +16,7 @@ impl Imap { // First of all, debounce to once per minute: let mut last_scan = context.last_full_folder_scan.lock().await; if let Some(last_scan) = *last_scan { - let elapsed_secs = last_scan.elapsed().as_secs(); + let elapsed_secs = time_elapsed(&last_scan).as_secs(); let debounce_secs = context .get_config_u64(Config::ScanAllFoldersDebounceSecs) .await?; @@ -93,7 +94,7 @@ impl Imap { .await?; } - last_scan.replace(Instant::now()); + last_scan.replace(tools::Time::now()); Ok(true) } diff --git a/src/key.rs b/src/key.rs index c5d50f853..865f53523 100644 --- a/src/key.rs +++ b/src/key.rs @@ -18,7 +18,7 @@ use crate::constants::KeyGenType; use crate::context::Context; use crate::log::LogExt; use crate::pgp::KeyPair; -use crate::tools::EmailAddress; +use crate::tools::{self, time_elapsed, EmailAddress}; /// Convenience trait for working with keys. /// @@ -204,7 +204,7 @@ async fn generate_keypair(context: &Context) -> Result { match load_keypair(context, &addr).await? { Some(key_pair) => Ok(key_pair), None => { - let start = std::time::SystemTime::now(); + let start = tools::Time::now(); let keytype = KeyGenType::from_i32(context.get_config_int(Config::KeyGenType).await?) .unwrap_or_default(); info!(context, "Generating keypair with type {}", keytype); @@ -216,7 +216,7 @@ async fn generate_keypair(context: &Context) -> Result { info!( context, "Keypair generated in {:.3}s.", - start.elapsed().unwrap_or_default().as_secs() + time_elapsed(&start).as_secs(), ); Ok(keypair) } diff --git a/src/quota.rs b/src/quota.rs index 6fa272a88..8469f6a69 100644 --- a/src/quota.rs +++ b/src/quota.rs @@ -12,7 +12,7 @@ use crate::imap::scan_folders::get_watched_folders; use crate::imap::session::Session as ImapSession; use crate::imap::Imap; use crate::message::{Message, Viewtype}; -use crate::tools::time; +use crate::tools; use crate::{stock_str, EventType}; /// warn about a nearly full mailbox after this usage percentage is reached. @@ -40,8 +40,8 @@ pub struct QuotaInfo { /// set to `Ok()` for valid quota information. pub(crate) recent: Result>>, - /// Timestamp when structure was modified. - pub(crate) modified: i64, + /// When the structure was modified. + pub(crate) modified: tools::Time, } async fn get_unique_quota_roots_and_usage( @@ -151,7 +151,7 @@ impl Context { *self.quota.write().await = Some(QuotaInfo { recent: quota, - modified: time(), + modified: tools::Time::now(), }); self.emit_event(EventType::ConnectivityChanged); diff --git a/src/scheduler.rs b/src/scheduler.rs index 6a00fb5ce..038fd0c31 100644 --- a/src/scheduler.rs +++ b/src/scheduler.rs @@ -2,6 +2,7 @@ use std::cmp; use std::iter::{self, once}; use std::num::NonZeroUsize; use std::sync::atomic::Ordering; +use std::time::Duration; use anyhow::{bail, Context as _, Error, Result}; use async_channel::{self as channel, Receiver, Sender}; @@ -24,7 +25,7 @@ use crate::log::LogExt; use crate::message::MsgId; use crate::smtp::{send_smtp_messages, Smtp}; use crate::sql; -use crate::tools::{duration_to_str, maybe_add_time_based_warnings, time}; +use crate::tools::{self, duration_to_str, maybe_add_time_based_warnings, time, time_elapsed}; pub(crate) mod connectivity; @@ -398,7 +399,7 @@ async fn inbox_loop( let quota = ctx.quota.read().await; quota .as_ref() - .filter(|quota| quota.modified + 60 > time()) + .filter(|quota| time_elapsed("a.modified) > Duration::from_secs(60)) .is_none() }; @@ -785,7 +786,7 @@ async fn smtp_loop( // again, we increase the timeout exponentially, in order not to do lots of // unnecessary retries. if let Some(t) = timeout { - let now = tokio::time::Instant::now(); + let now = tools::Time::now(); info!( ctx, "smtp has messages to retry, planning to retry {} seconds later", t, @@ -796,7 +797,7 @@ async fn smtp_loop( }) .await .unwrap_or_default(); - let slept = (tokio::time::Instant::now() - now).as_secs(); + let slept = time_elapsed(&now).as_secs(); timeout = Some(cmp::max( t, slept.saturating_add(rand::thread_rng().gen_range((slept / 2)..=slept)), diff --git a/src/smtp.rs b/src/smtp.rs index a503df045..a7ab86c9e 100644 --- a/src/smtp.rs +++ b/src/smtp.rs @@ -2,7 +2,7 @@ pub mod send; -use std::time::{Duration, SystemTime}; +use std::time::Duration; use anyhow::{bail, format_err, Context as _, Error, Result}; use async_smtp::response::{Category, Code, Detail}; @@ -28,6 +28,7 @@ use crate::scheduler::connectivity::ConnectivityStore; use crate::socks::Socks5Config; use crate::sql; use crate::stock_str::unencrypted_email; +use crate::tools::{self, time_elapsed}; /// SMTP connection, write and read timeout. const SMTP_TIMEOUT: Duration = Duration::from_secs(60); @@ -43,7 +44,7 @@ pub(crate) struct Smtp { /// Timestamp of last successful send/receive network interaction /// (eg connect or send succeeded). On initialization and disconnect /// it is set to None. - last_success: Option, + last_success: Option, pub(crate) connectivity: ConnectivityStore, @@ -72,11 +73,7 @@ impl Smtp { /// have been successfully used the last 60 seconds pub fn has_maybe_stale_connection(&self) -> bool { if let Some(last_success) = self.last_success { - SystemTime::now() - .duration_since(last_success) - .unwrap_or_default() - .as_secs() - > 60 + time_elapsed(&last_success).as_secs() > 60 } else { false } @@ -336,7 +333,7 @@ impl Smtp { } self.transport = Some(transport); - self.last_success = Some(SystemTime::now()); + self.last_success = Some(tools::Time::now()); context.emit_event(EventType::SmtpConnected(format!( "SMTP-LOGIN as {} ok", diff --git a/src/smtp/send.rs b/src/smtp/send.rs index 7adb59765..a12c20f1a 100644 --- a/src/smtp/send.rs +++ b/src/smtp/send.rs @@ -6,6 +6,7 @@ use super::Smtp; use crate::config::Config; use crate::context::Context; use crate::events::EventType; +use crate::tools; pub type Result = std::result::Result; @@ -55,7 +56,7 @@ impl Smtp { format!("Message len={message_len_bytes} was SMTP-sent to {recipients_display}"); info!(context, "{info_msg}."); context.emit_event(EventType::SmtpMessageSent(info_msg)); - self.last_success = Some(std::time::SystemTime::now()); + self.last_success = Some(tools::Time::now()); } else { warn!( context, diff --git a/src/tools.rs b/src/tools.rs index 20a73eff5..7563ab139 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -9,6 +9,13 @@ use std::io::{Cursor, Write}; use std::mem; use std::path::{Path, PathBuf}; use std::str::from_utf8; +// If a time value doesn't need to be sent to another host, saved to the db or otherwise used across +// program restarts, a monotonically nondecreasing clock (`Instant`) should be used. But as +// `Instant` may use `libc::clock_gettime(CLOCK_MONOTONIC)`, e.g. on Android, and does not advance +// while being in deep sleep mode, we use `SystemTime` instead, but add an alias for it to document +// why `Instant` isn't used in those places. Also this can help to switch to another clock impl if +// we find any. +pub use std::time::SystemTime as Time; use std::time::{Duration, SystemTime}; use anyhow::{bail, Context as _, Result}; @@ -482,6 +489,10 @@ pub(crate) fn time() -> i64 { .as_secs() as i64 } +pub(crate) fn time_elapsed(time: &Time) -> Duration { + time.elapsed().unwrap_or_default() +} + /// Struct containing all mailto information #[derive(Debug, Default, Eq, PartialEq)] pub struct MailTo {