From c3a53cefb09bd0005151b64d959bdc6d37543ea2 Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Mon, 18 Nov 2019 22:43:08 +0100 Subject: [PATCH 1/2] fix time smearing if two messages have the same time, this results i all kinds of sorting failures, esp. in non-delta-muas and on forwarding messages. so, no two messages sent out from delta have the same timestamp. normally, this is no problem, but when things are sent too fast, eg. on forwarding, we lend us some time from the future. however, all this did not work because we forgot to write back the modified time, this is fixed by this commit, as well as some cleanup. --- src/context.rs | 4 ++-- src/dc_tools.rs | 61 +++++++++++++++++++++++++++++++++++++------------ 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/src/context.rs b/src/context.rs index 13e7eec50..7322aecf3 100644 --- a/src/context.rs +++ b/src/context.rs @@ -57,7 +57,7 @@ pub struct Context { pub os_name: Option, pub cmdline_sel_chat_id: Arc>, pub bob: Arc>, - pub last_smeared_timestamp: Arc>, + pub last_smeared_timestamp: RwLock, pub running_state: Arc>, /// Mutex to avoid generating the key for the user more than once. pub generating_key_mutex: Mutex<()>, @@ -130,7 +130,7 @@ impl Context { smtp_state: Arc::new((Mutex::new(Default::default()), Condvar::new())), oauth2_critical: Arc::new(Mutex::new(())), bob: Arc::new(RwLock::new(Default::default())), - last_smeared_timestamp: Arc::new(RwLock::new(0)), + last_smeared_timestamp: RwLock::new(0), cmdline_sel_chat_id: Arc::new(RwLock::new(0)), sentbox_thread: Arc::new(RwLock::new(JobThread::new( "SENTBOX", diff --git a/src/dc_tools.rs b/src/dc_tools.rs index 091aec493..1d722d8bd 100644 --- a/src/dc_tools.rs +++ b/src/dc_tools.rs @@ -1,7 +1,7 @@ //! Some tools and enhancements to the used libraries, there should be //! no references to Context and other "larger" entities here. -use core::cmp::max; +use core::cmp::{max, min}; use std::borrow::Cow; use std::ffi::{CStr, CString}; use std::path::{Path, PathBuf}; @@ -166,10 +166,12 @@ pub(crate) fn dc_gm2local_offset() -> i64 { } /* timesmearing */ +const MAX_SECONDS_TO_LEND_FROM_FUTURE: i64 = 5; + pub(crate) fn dc_smeared_time(context: &Context) -> i64 { /* function returns a corrected time(NULL) */ let mut now = time(); - let ts = *context.last_smeared_timestamp.clone().read().unwrap(); + let ts = *context.last_smeared_timestamp.read().unwrap(); if ts >= now { now = ts + 1; } @@ -181,28 +183,29 @@ pub(crate) fn dc_create_smeared_timestamp(context: &Context) -> i64 { let now = time(); let mut ret = now; - let ts = *context.last_smeared_timestamp.clone().write().unwrap(); - if ret <= ts { - ret = ts + 1; - if ret - now > 5 { - ret = now + 5 + let mut last_smeared_timestamp = context.last_smeared_timestamp.write().unwrap(); + if ret <= *last_smeared_timestamp { + ret = *last_smeared_timestamp + 1; + if ret - now > MAX_SECONDS_TO_LEND_FROM_FUTURE { + ret = now + MAX_SECONDS_TO_LEND_FROM_FUTURE } } + *last_smeared_timestamp = ret; ret } pub(crate) fn dc_create_smeared_timestamps(context: &Context, count: usize) -> i64 { /* get a range to timestamps that can be used uniquely */ let now = time(); - let start = now + (if count < 5 { count } else { 5 }) as i64 - count as i64; + let count = count as i64; + let mut start = now + min(count, MAX_SECONDS_TO_LEND_FROM_FUTURE) - count; - let ts = *context.last_smeared_timestamp.clone().write().unwrap(); - if ts + 1 > start { - ts + 1 - } else { - start - } + let mut last_smeared_timestamp = context.last_smeared_timestamp.write().unwrap(); + start = max(*last_smeared_timestamp + 1, start); + + *last_smeared_timestamp = start + count - 1; + start } /* Message-ID tools */ @@ -1355,4 +1358,34 @@ mod tests { let listflags: u32 = DC_GCL_VERIFIED_ONLY.try_into().unwrap(); assert!(listflags_has(listflags, DC_GCL_ADD_SELF) == false); } + + #[test] + fn test_create_smeared_timestamp() { + let t = dummy_context(); + assert_ne!( + dc_create_smeared_timestamp(&t.ctx), + dc_create_smeared_timestamp(&t.ctx) + ); + assert!( + dc_create_smeared_timestamp(&t.ctx) + >= SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .unwrap() + .as_secs() as i64 + ); + } + + #[test] + fn test_create_smeared_timestamps() { + let t = dummy_context(); + let count = MAX_SECONDS_TO_LEND_FROM_FUTURE - 1; + let start = dc_create_smeared_timestamps(&t.ctx, count as usize); + let next = dc_smeared_time(&t.ctx); + assert!((start + count - 1) < next); + + let count = MAX_SECONDS_TO_LEND_FROM_FUTURE + 30; + let start = dc_create_smeared_timestamps(&t.ctx, count as usize); + let next = dc_smeared_time(&t.ctx); + assert!((start + count - 1) < next); + } } From f7047bbf51de77e1f0b7d4c0c65cb45872318bde Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Tue, 19 Nov 2019 22:52:24 +0100 Subject: [PATCH 2/2] add detailed comments about time-smearing --- src/dc_tools.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/dc_tools.rs b/src/dc_tools.rs index 1d722d8bd..4eefe97b6 100644 --- a/src/dc_tools.rs +++ b/src/dc_tools.rs @@ -165,11 +165,24 @@ pub(crate) fn dc_gm2local_offset() -> i64 { lt.offset().local_minus_utc() as i64 } -/* timesmearing */ +// timesmearing +// - as e-mails typically only use a second-based-resolution for timestamps, +// the order of two mails sent withing one second is unclear. +// this is bad eg. when forwarding some messages from a chat - +// these messages will appear at the recipient easily out of order. +// - we work around this issue by not sending out two mails with the same timestamp. +// - for this purpose, in short, we track the last timestamp used in `last_smeared_timestamp` +// when another timestamp is needed in the same second, we use `last_smeared_timestamp+1` +// - after some moments without messages sent out, +// `last_smeared_timestamp` is again in sync with the normal time. +// - however, we do not do all this for the far future, +// but at max `MAX_SECONDS_TO_LEND_FROM_FUTURE` const MAX_SECONDS_TO_LEND_FROM_FUTURE: i64 = 5; +// returns the currently smeared timestamp, +// may be used to check if call to dc_create_smeared_timestamp() is needed or not. +// the returned timestamp MUST NOT be used to be sent out or saved in the database! pub(crate) fn dc_smeared_time(context: &Context) -> i64 { - /* function returns a corrected time(NULL) */ let mut now = time(); let ts = *context.last_smeared_timestamp.read().unwrap(); if ts >= now { @@ -179,6 +192,7 @@ pub(crate) fn dc_smeared_time(context: &Context) -> i64 { now } +// returns a timestamp that is guaranteed to be unique. pub(crate) fn dc_create_smeared_timestamp(context: &Context) -> i64 { let now = time(); let mut ret = now; @@ -195,8 +209,10 @@ pub(crate) fn dc_create_smeared_timestamp(context: &Context) -> i64 { ret } +// creates `count` timestamps that are guaranteed to be unique. +// the frist created timestamps is returned directly, +// get the other timestamps just by adding 1..count-1 pub(crate) fn dc_create_smeared_timestamps(context: &Context, count: usize) -> i64 { - /* get a range to timestamps that can be used uniquely */ let now = time(); let count = count as i64; let mut start = now + min(count, MAX_SECONDS_TO_LEND_FROM_FUTURE) - count;