hold smtp lock during the full send action

Hopefully closes #331

.circleci/config.yml

@@ -13,7 +13,7 @@ restore-workspace: &restore-workspace
 restore-cache: &restore-cache
   restore_cache:
     keys:
-      - cargo-v0-{{ checksum "rust-toolchain" }}-{{ checksum "Cargo.toml" }}-{{ checksum "Cargo.lock" }}-{{ arch }}
+      - cargo-v1-{{ checksum "rust-toolchain" }}-{{ checksum "Cargo.toml" }}-{{ checksum "Cargo.lock" }}-{{ arch }}
       - repo-source-{{ .Branch }}-{{ .Revision }}
 
 commands:
@@ -53,10 +53,11 @@ jobs:
           command: cargo generate-lockfile
       - restore_cache:
           keys:
-            - cargo-v0-{{ checksum "rust-toolchain" }}-{{ checksum "Cargo.toml" }}-{{ checksum "Cargo.lock" }}-{{ arch }}
+            - cargo-v1-{{ checksum "rust-toolchain" }}-{{ checksum "Cargo.toml" }}-{{ checksum "Cargo.lock" }}-{{ arch }}
       - run: rustup install $(cat rust-toolchain)
       - run: rustup default $(cat rust-toolchain)
       - run: rustup component add --toolchain $(cat rust-toolchain) rustfmt
+      - run: rustup component add --toolchain $(cat rust-toolchain) clippy-preview
       - run: cargo update
       - run: cargo fetch
       - run: rustc +stable --version
@@ -67,7 +68,7 @@ jobs:
           paths:
             - crate
       - save_cache:
-          key: cargo-v0-{{ checksum "rust-toolchain" }}-{{ checksum "Cargo.toml" }}-{{ checksum "Cargo.lock" }}-{{ arch }}
+          key: cargo-v1-{{ checksum "rust-toolchain" }}-{{ checksum "Cargo.toml" }}-{{ checksum "Cargo.lock" }}-{{ arch }}
           paths:
             - "~/.cargo"
             - "~/.rustup"
@@ -160,6 +161,15 @@ jobs:
       - run: ls -laR workspace
       - run: ci_scripts/ci_upload.sh workspace/py-docs workspace/wheelhouse
 
+  clippy:
+    executor: default
+    steps:
+      - *restore-workspace
+      - *restore-cache
+      - run:
+          name: Run cargo clippy
+          command: cargo clippy --all
+
 
 workflows:
   version: 2.1
@@ -175,6 +185,10 @@ workflows:
           requires:
             - cargo_fetch
 
+      - clippy:
+          requires:
+            - cargo_fetch
+
       # Linux Desktop 64bit
       - test_x86_64-unknown-linux-gnu:
           requires:

ci_scripts/run_all.sh

@@ -39,7 +39,7 @@ if [ -n "$TESTS" ]; then
     # run tox
     # XXX we don't run liveconfig tests because they hang sometimes
     # see https://github.com/deltachat/deltachat-core-rust/issues/331
-    unset DCC_PY_LIVECONFIG
+    # unset DCC_PY_LIVECONFIG
 
     tox --workdir "$TOXWORKDIR" -e lint,py27,py35,py36,py37,auditwheels
     popd

examples/repl/main.rs

@@ -3,7 +3,6 @@
 //!
 //! Usage:  cargo run --example repl --release -- <databasefile>
 //! All further options can be set using the set-command (type ? for help).
-#![feature(ptr_cast)]
 
 #[macro_use]
 extern crate deltachat;

rust-toolchain

@@ -1 +1 @@
-nightly-2019-07-10
+nightly-2019-08-13

src/dc_array.rs

@@ -403,5 +403,4 @@ mod tests {
         }
         unsafe { dc_array_get_id(arr, 1000) };
     }
-
 }

src/dc_chat.rs

@@ -1307,7 +1307,9 @@ pub unsafe fn dc_get_next_media(
         }
     }
 
-    dc_array_unref(list);
+    if !list.is_null() {
+        dc_array_unref(list);
+    }
     dc_msg_unref(msg);
     ret_msg_id
 }

src/dc_configure.rs

@@ -19,8 +19,8 @@ macro_rules! progress {
             Event::CONFIGURE_PROGRESS,
             (if $progress < 1 {
                 1
-            } else if $progress > 999 {
-                999
+            } else if $progress > 1000 {
+                1000
             } else {
                 $progress
             }) as uintptr_t,

src/dc_job.rs

@@ -352,18 +352,15 @@ unsafe fn dc_job_do_DC_JOB_SEND(context: &Context, job: &mut dc_job_t) {
                 if ok_to_continue1 {
                     /* send message */
                     let body = std::slice::from_raw_parts(buf as *const u8, buf_bytes).to_vec();
-                    if 0 == context
-                        .smtp
-                        .lock()
-                        .unwrap()
-                        .send(context, recipients_list, body)
-                    {
-                        context.smtp.lock().unwrap().disconnect();
-                        dc_job_try_again_later(
-                            job,
-                            -1i32,
-                            (*&mut context.smtp.clone().lock().unwrap()).error,
-                        );
+
+                    // hold the smtp lock during sending of a job and
+                    // its ok/error response processing. Note that if a message
+                    // was sent we need to mark it in the database as we
+                    // otherwise might send it twice.
+                    let mut sock = context.smtp.lock().unwrap();
+                    if 0 == sock.send(context, recipients_list, body) {
+                        sock.disconnect();
+                        dc_job_try_again_later(job, -1i32, sock.error);
                     } else {
                         dc_delete_file(context, filename_s);
                         if 0 != job.foreign_id {

src/dc_receive_imf.rs

@@ -1,3 +1,5 @@
+use itertools::join;
+
 use mmime::mailimf::*;
 use mmime::mailimf_types::*;
 use mmime::mailmime::*;
@@ -1504,8 +1506,12 @@ unsafe fn create_or_lookup_adhoc_group(
                    ret_chat_id_blocked: *mut libc::c_int,
                    chat_id: u32,
                    chat_id_blocked: i32| {
-        dc_array_unref(member_ids);
-        dc_array_unref(chat_ids);
+        if !member_ids.is_null() {
+            dc_array_unref(member_ids);
+        }
+        if !chat_ids.is_null() {
+            dc_array_unref(chat_ids);
+        }
         free(chat_ids_str as *mut libc::c_void);
         free(grpid as *mut libc::c_void);
         free(grpname as *mut libc::c_void);
@@ -1740,9 +1746,8 @@ unsafe fn search_chat_ids_by_contact_ids(
     unsorted_contact_ids: *const dc_array_t,
 ) -> *mut dc_array_t {
     /* searches chat_id's by the given contact IDs, may return zero, one or more chat_id's */
-    let contact_ids: *mut dc_array_t = dc_array_new(23 as size_t);
-    let mut contact_ids_str: *mut libc::c_char = 0 as *mut libc::c_char;
-    let chat_ids: *mut dc_array_t = dc_array_new(23 as size_t);
+    let mut contact_ids = Vec::with_capacity(23);
+    let mut chat_ids = Vec::with_capacity(23);
 
     /* copy array, remove duplicates and SELF, sort by ID */
     let mut i: libc::c_int;
@@ -1751,22 +1756,18 @@ unsafe fn search_chat_ids_by_contact_ids(
         i = 0;
         while i < iCnt {
             let curr_id: uint32_t = dc_array_get_id(unsorted_contact_ids, i as size_t);
-            if curr_id != 1 as libc::c_uint
-                && !dc_array_search_id(contact_ids, curr_id, 0 as *mut size_t)
-            {
-                dc_array_add_id(contact_ids, curr_id);
+            if curr_id != 1 as libc::c_uint && !contact_ids.contains(&curr_id) {
+                contact_ids.push(curr_id);
             }
             i += 1
         }
-        if !(dc_array_get_cnt(contact_ids) == 0) {
-            (*contact_ids).sort_ids();
-            contact_ids_str =
-                dc_array_get_string(contact_ids, b",\x00" as *const u8 as *const libc::c_char);
-
+        if !contact_ids.is_empty() {
+            contact_ids.sort();
+            let contact_ids_str = join(contact_ids.iter().map(|x| x.to_string()), ",");
             context.sql.query_map(
                 format!(
                     "SELECT DISTINCT cc.chat_id, cc.contact_id  FROM chats_contacts cc  LEFT JOIN chats c ON c.id=cc.chat_id  WHERE cc.chat_id IN(SELECT chat_id FROM chats_contacts WHERE contact_id IN({}))   AND c.type=120   AND cc.contact_id!=1 ORDER BY cc.chat_id, cc.contact_id;",
-                    as_str(contact_ids_str)
+                    contact_ids_str
                 ),
                 params![],
                 |row| Ok((row.get::<_, i32>(0)?, row.get::<_, i32>(1)?)),
@@ -1778,32 +1779,30 @@ unsafe fn search_chat_ids_by_contact_ids(
                     for row in rows {
                         let (chat_id, contact_id) = row?;
                         if chat_id as u32 != last_chat_id {
-                            if matches == dc_array_get_cnt(contact_ids) && mismatches == 0 {
-                                dc_array_add_id(chat_ids, last_chat_id);
+                            if matches == contact_ids.len() && mismatches == 0 {
+                                chat_ids.push(last_chat_id);
                             }
                             last_chat_id = chat_id as u32;
                             matches = 0;
                             mismatches = 0;
                         }
-                        if contact_id as u32 == dc_array_get_id(contact_ids, matches as size_t) {
+                        if contact_id as u32 == contact_ids[matches] {
                             matches += 1;
                         } else {
                             mismatches += 1;
                         }
                     }
 
-                    if matches == dc_array_get_cnt(contact_ids) && mismatches == 0 {
-                        dc_array_add_id(chat_ids, last_chat_id);
+                    if matches == contact_ids.len() && mismatches == 0 {
+                        chat_ids.push(last_chat_id);
                     }
                 Ok(())
                 }
             ).unwrap(); // TODO: better error handling
         }
     }
-    free(contact_ids_str as *mut libc::c_void);
-    dc_array_unref(contact_ids);
 
-    chat_ids
+    dc_array_t::from(chat_ids).into_raw()
 }
 
 unsafe fn check_verified_properties(

src/imap.rs

@@ -1,6 +1,9 @@
 use std::ffi::CString;
 use std::net;
-use std::sync::{Arc, Condvar, Mutex, RwLock};
+use std::sync::{
+    atomic::{AtomicBool, Ordering},
+    Arc, Condvar, Mutex, RwLock,
+};
 use std::time::{Duration, SystemTime};
 
 use crate::constants::*;
@@ -35,6 +38,8 @@ pub struct Imap {
     session: Arc<Mutex<Option<Session>>>,
     stream: Arc<RwLock<Option<net::TcpStream>>>,
     connected: Arc<Mutex<bool>>,
+
+    should_reconnect: AtomicBool,
 }
 
 struct OAuth2 {
@@ -313,7 +318,6 @@ pub struct ImapConfig {
     pub selected_folder: Option<String>,
     pub selected_mailbox: Option<imap::types::Mailbox>,
     pub selected_folder_needs_expunge: bool,
-    pub should_reconnect: bool,
     pub can_idle: bool,
     pub has_xlist: bool,
     pub imap_delimiter: char,
@@ -332,7 +336,6 @@ impl Default for ImapConfig {
             selected_folder: None,
             selected_mailbox: None,
             selected_folder_needs_expunge: false,
-            should_reconnect: false,
             can_idle: false,
             has_xlist: false,
             imap_delimiter: '.',
@@ -360,6 +363,7 @@ impl Imap {
             precheck_imf,
             receive_imf,
             connected: Arc::new(Mutex::new(false)),
+            should_reconnect: AtomicBool::new(false),
         }
     }
 
@@ -368,7 +372,7 @@ impl Imap {
     }
 
     pub fn should_reconnect(&self) -> bool {
-        self.config.read().unwrap().should_reconnect
+        self.should_reconnect.load(Ordering::Relaxed)
     }
 
     fn setup_handle_if_needed(&self, context: &Context) -> bool {
@@ -381,7 +385,7 @@ impl Imap {
         }
 
         if self.is_connected() && self.stream.read().unwrap().is_some() {
-            self.config.write().unwrap().should_reconnect = false;
+            self.should_reconnect.store(false, Ordering::Relaxed);
             return true;
         }
 
@@ -451,7 +455,7 @@ impl Imap {
             }
         };
 
-        self.config.write().unwrap().should_reconnect = false;
+        self.should_reconnect.store(false, Ordering::Relaxed);
 
         match login_res {
             Ok((session, stream)) => {
@@ -550,14 +554,12 @@ impl Imap {
             return false;
         }
 
-        let teardown: bool;
-
-        match &mut *self.session.lock().unwrap() {
+        let (teardown, can_idle, has_xlist) = match &mut *self.session.lock().unwrap() {
             Some(ref mut session) => {
                 if let Ok(caps) = session.capabilities() {
                     if !context.sql.is_open() {
                         warn!(context, 0, "IMAP-LOGIN as {} ok but ABORTING", lp.mail_user,);
-                        teardown = true;
+                        (true, false, false)
                     } else {
                         let can_idle = caps.has("IDLE");
                         let has_xlist = caps.has("XLIST");
@@ -574,24 +576,23 @@ impl Imap {
                             lp.mail_user,
                             caps_list,
                         );
-                        self.config.write().unwrap().can_idle = can_idle;
-                        self.config.write().unwrap().has_xlist = has_xlist;
-                        *self.connected.lock().unwrap() = true;
-                        teardown = false;
+                        (false, can_idle, has_xlist)
                     }
                 } else {
-                    teardown = true;
+                    (true, false, false)
                 }
             }
-            None => {
-                teardown = true;
-            }
-        }
+            None => (true, false, false),
+        };
+
         if teardown {
             self.unsetup_handle(context);
             self.free_connect_params();
             false
         } else {
+            self.config.write().unwrap().can_idle = can_idle;
+            self.config.write().unwrap().has_xlist = has_xlist;
+            *self.connected.lock().unwrap() = true;
             true
         }
     }
@@ -689,9 +690,8 @@ impl Imap {
                             err
                         );
 
-                        let mut config = self.config.write().unwrap();
-                        config.selected_folder = None;
-                        config.should_reconnect = true;
+                        self.config.write().unwrap().selected_folder = None;
+                        self.should_reconnect.store(true, Ordering::Relaxed);
                         return 0;
                     }
                 }
@@ -777,7 +777,7 @@ impl Imap {
                 match session.fetch(set, PREFETCH_FLAGS) {
                     Ok(list) => list,
                     Err(_err) => {
-                        self.config.write().unwrap().should_reconnect = true;
+                        self.should_reconnect.store(true, Ordering::Relaxed);
                         info!(
                             context,
                             0,
@@ -822,7 +822,7 @@ impl Imap {
             match session.uid_fetch(set, PREFETCH_FLAGS) {
                 Ok(list) => list,
                 Err(err) => {
-                    eprintln!("fetch err: {:?}", err);
+                    warn!(context, 0, "failed to fetch uids: {}", err);
                     return 0;
                 }
             }
@@ -928,15 +928,13 @@ impl Imap {
             return 0;
         }
 
-        let mut retry_later = false;
-
         let set = format!("{}", server_uid);
 
         let msgs = if let Some(ref mut session) = &mut *self.session.lock().unwrap() {
             match session.uid_fetch(set, BODY_FLAGS) {
                 Ok(msgs) => msgs,
                 Err(err) => {
-                    self.config.write().unwrap().should_reconnect = true;
+                    self.should_reconnect.store(true, Ordering::Relaxed);
                     warn!(
                         context,
                         0,
@@ -946,17 +944,11 @@ impl Imap {
                         self.should_reconnect(),
                         err
                     );
-
-                    if self.should_reconnect() {
-                        // maybe we should also retry on other errors, however, we should check this carefully, as this may result in a dead lock!
-                        retry_later = true;
-                    }
-
-                    return if retry_later { 0 } else { 1 };
+                    return 0;
                 }
             }
         } else {
-            return if retry_later { 0 } else { 1 };
+            return 1;
         };
 
         if msgs.is_empty() {
@@ -1004,11 +996,7 @@ impl Imap {
             }
         }
 
-        if retry_later {
-            0
-        } else {
-            1
-        }
+        1
     }
 
     pub fn idle(&self, context: &Context) {
@@ -1073,7 +1061,7 @@ impl Imap {
                         context,
                         0, "IMAP-IDLE wait cancelled, we will reconnect soon."
                     );
-                    self.config.write().unwrap().should_reconnect = true;
+                    self.should_reconnect.store(true, Ordering::Relaxed);
                 }
                 _ => {
                     warn!(context, 0, "IMAP-IDLE returns unknown value: {}", err);

src/lib.rs

@@ -1,4 +1,9 @@
-#![feature(c_variadic, ptr_wrapping_offset_from, ptr_cast)]
+#![deny(clippy::correctness)]
+// TODO: make all of these errors, such that clippy actually passes.
+#![warn(clippy::all, clippy::perf, clippy::not_unsafe_ptr_arg_deref)]
+// This is nice, but for now just annoying.
+#![allow(clippy::unreadable_literal)]
+#![feature(ptr_wrapping_offset_from)]
 
 #[macro_use]
 extern crate failure_derive;