Merge pull request #856 from deltachat/fix-time-smearing

fix time smearing

src/context.rs

@@ -57,7 +57,7 @@ pub struct Context {
     pub os_name: Option<String>,
     pub cmdline_sel_chat_id: Arc<RwLock<u32>>,
     pub bob: Arc<RwLock<BobStatus>>,
-    pub last_smeared_timestamp: Arc<RwLock<i64>>,
+    pub last_smeared_timestamp: RwLock<i64>,
     pub running_state: Arc<RwLock<RunningState>>,
     /// 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",

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};
@@ -165,11 +165,26 @@ 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.clone().read().unwrap();
+    let ts = *context.last_smeared_timestamp.read().unwrap();
     if ts >= now {
         now = ts + 1;
     }
@@ -177,32 +192,36 @@ 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;
 
-    let ts = *context.last_smeared_timestamp.clone().write().unwrap();
+    let mut last_smeared_timestamp = context.last_smeared_timestamp.write().unwrap();
-    if ret <= ts {
+    if ret <= *last_smeared_timestamp {
-        ret = ts + 1;
+        ret = *last_smeared_timestamp + 1;
-        if ret - now > 5 {
+        if ret - now > MAX_SECONDS_TO_LEND_FROM_FUTURE {
-            ret = now + 5
+            ret = now + MAX_SECONDS_TO_LEND_FROM_FUTURE
         }
     }
 
+    *last_smeared_timestamp = ret;
     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 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();
+    let mut last_smeared_timestamp = context.last_smeared_timestamp.write().unwrap();
-    if ts + 1 > start {
+    start = max(*last_smeared_timestamp + 1, start);
-        ts + 1
+
-    } else {
+    *last_smeared_timestamp = start + count - 1;
-        start
+    start
-    }
 }
 
 /* Message-ID tools */
@@ -1355,4 +1374,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);
+    }
 }