From 131f54fbf1c2ab4690ea8ddb8152b91c8b7f9600 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 11 Nov 2019 14:07:11 +0100 Subject: [PATCH 1/4] make select_folder return ImapActionResult's and early-return from idle if there is no selected folder --- src/imap.rs | 104 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 40 deletions(-) diff --git a/src/imap.rs b/src/imap.rs index 64db44093..2d13662a1 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -396,12 +396,16 @@ impl Imap { }) } - async fn select_folder>(&self, context: &Context, folder: Option) -> usize { + async fn select_folder>( + &self, + context: &Context, + folder: Option, + ) -> ImapActionResult { if self.session.lock().await.is_none() { let mut cfg = self.config.write().await; cfg.selected_folder = None; cfg.selected_folder_needs_expunge = false; - return 0; + return ImapActionResult::Failed; } // if there is a new folder and the new folder is equal to the selected one, there's nothing to do. @@ -409,7 +413,7 @@ impl Imap { if let Some(ref folder) = folder { if let Some(ref selected_folder) = self.config.read().await.selected_folder { if folder.as_ref() == selected_folder { - return 1; + return ImapActionResult::AlreadyDone; } } } @@ -429,11 +433,11 @@ impl Imap { } Err(err) => { warn!(context, "failed to close session: {:?}", err); - return 0; + return ImapActionResult::Failed; } } } else { - return 0; + return ImapActionResult::Failed; } } self.config.write().await.selected_folder_needs_expunge = false; @@ -449,7 +453,7 @@ impl Imap { config.selected_mailbox = Some(mailbox); } Err(err) => { - info!( + warn!( context, "Cannot select folder: {}; {:?}.", folder.as_ref(), @@ -458,7 +462,7 @@ impl Imap { self.config.write().await.selected_folder = None; self.should_reconnect.store(true, Ordering::Relaxed); - return 0; + return ImapActionResult::Failed; } } } else { @@ -466,7 +470,7 @@ impl Imap { } } - 1 + ImapActionResult::Success } fn get_config_last_seen_uid>(&self, context: &Context, folder: S) -> (u32, u32) { @@ -502,13 +506,13 @@ impl Imap { return 0; } - if self.select_folder(context, Some(&folder)).await == 0 { + let res = self.select_folder(context, Some(&folder)).await; + if res == ImapActionResult::Failed || res == ImapActionResult::RetryLater { info!( context, "Cannot select folder \"{}\" for fetching.", folder.as_ref() ); - return 0; } @@ -748,6 +752,9 @@ impl Imap { pub fn idle(&self, context: &Context) { task::block_on(async move { + if self.config.read().await.selected_folder.is_none() { + return; + } if !self.config.read().await.can_idle { self.fake_idle(context).await; return; @@ -756,11 +763,18 @@ impl Imap { self.setup_handle_if_needed(context); let watch_folder = self.config.read().await.watch_folder.clone(); - if self.select_folder(context, watch_folder.as_ref()).await == 0 { - warn!(context, "IMAP-IDLE not setup."); + match self.select_folder(context, watch_folder.as_ref()).await { + ImapActionResult::Success | ImapActionResult::AlreadyDone => {} - self.fake_idle(context).await; - return; + ImapActionResult::Failed | ImapActionResult::RetryLater => { + warn!( + context, + "idle select_folder failed {:?}", + watch_folder.as_ref() + ); + self.fake_idle(context).await; + return; + } } let session = self.session.lock().await.take(); @@ -999,14 +1013,16 @@ impl Imap { return Some(ImapActionResult::RetryLater); } } - if self.select_folder(context, Some(&folder)).await == 0 { - warn!( - context, - "Cannot select folder {} for preparing IMAP operation", folder - ); - Some(ImapActionResult::RetryLater) - } else { - None + match self.select_folder(context, Some(&folder)).await { + ImapActionResult::Success | ImapActionResult::AlreadyDone => None, + res => { + warn!( + context, + "Cannot select folder {} for preparing IMAP operation", folder + ); + + Some(res) + } } }) } @@ -1225,26 +1241,34 @@ impl Imap { task::block_on(async move { info!(context, "emptying folder {}", folder); - if folder.is_empty() || self.select_folder(context, Some(&folder)).await == 0 { - warn!(context, "Cannot select folder '{}' for emptying", folder); + if !folder.is_empty() { + warn!(context, "cannot perform empty, folder not set"); return; } - - if !self - .add_flag_finalized_with_set(context, SELECT_ALL, "\\Deleted") - .await - { - warn!(context, "Cannot empty folder {}", folder); - } else { - // we now trigger expunge to actually delete messages - self.config.write().await.selected_folder_needs_expunge = true; - if self.select_folder::(context, None).await == 0 { - warn!( - context, - "could not perform expunge on empty-marked folder {}", folder - ); - } else { - emit_event!(context, Event::ImapFolderEmptied(folder.to_string())); + match self.select_folder(context, Some(&folder)).await { + ImapActionResult::Success | ImapActionResult::AlreadyDone => { + if !self + .add_flag_finalized_with_set(context, SELECT_ALL, "\\Deleted") + .await + { + warn!(context, "Cannot empty folder {}", folder); + } else { + // we now trigger expunge to actually delete messages + self.config.write().await.selected_folder_needs_expunge = true; + if self.select_folder::(context, None).await + == ImapActionResult::Success + { + emit_event!(context, Event::ImapFolderEmptied(folder.to_string())); + } else { + warn!( + context, + "could not perform expunge on empty-marked folder {}", folder + ); + } + } + } + ImapActionResult::Failed | ImapActionResult::RetryLater => { + warn!(context, "could not select folder {}", folder); } } }); From a5a12d1f72180b595df701238fabfe414ac532a7 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 11 Nov 2019 15:33:19 +0100 Subject: [PATCH 2/4] * fix interrupt_idle by signalling "skip_next_idle_wait" to the potentially concurrently "fn idle" function --- src/imap.rs | 64 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 24 deletions(-) diff --git a/src/imap.rs b/src/imap.rs index 2d13662a1..024215632 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -45,6 +45,7 @@ pub struct Imap { session: Arc>>, connected: Arc>, interrupt: Arc>>, + skip_next_idle_wait: Arc>, should_reconnect: AtomicBool, } @@ -119,6 +120,7 @@ impl Imap { config: Arc::new(RwLock::new(ImapConfig::default())), interrupt: Arc::new(Mutex::new(None)), connected: Arc::new(Mutex::new(false)), + skip_next_idle_wait: Arc::new(Mutex::new(false)), should_reconnect: AtomicBool::new(false), } } @@ -496,24 +498,16 @@ impl Imap { } async fn fetch_from_single_folder>(&self, context: &Context, folder: S) -> usize { - if !self.is_connected().await { - info!( - context, - "Cannot fetch from \"{}\" - not connected.", - folder.as_ref() - ); - - return 0; - } - - let res = self.select_folder(context, Some(&folder)).await; - if res == ImapActionResult::Failed || res == ImapActionResult::RetryLater { - info!( - context, - "Cannot select folder \"{}\" for fetching.", - folder.as_ref() - ); - return 0; + match self.select_folder(context, Some(&folder)).await { + ImapActionResult::Failed | ImapActionResult::RetryLater => { + warn!( + context, + "Cannot select folder \"{}\" for fetching.", + folder.as_ref() + ); + return 0; + } + ImapActionResult::Success | ImapActionResult::AlreadyDone => {} } // compare last seen UIDVALIDITY against the current one @@ -789,8 +783,17 @@ impl Imap { let (idle_wait, interrupt) = handle.wait_with_timeout(timeout); *self.interrupt.lock().await = Some(interrupt); - let res = idle_wait.await; - info!(context, "Idle finished: {:?}", res); + if *self.skip_next_idle_wait.lock().await { + // interrupt_idle has happened before we + // provided self.interrupt + *self.skip_next_idle_wait.lock().await = false; + std::mem::drop(idle_wait); + info!(context, "Idle wait was skipped"); + } else { + info!(context, "Idle entering wait-on-remote state"); + let res = idle_wait.await; + info!(context, "Idle finished wait-on-remote: {:?}", res); + } match handle.done().await { Ok(session) => { *self.session.lock().await = Some(Session::Secure(session)); @@ -808,8 +811,18 @@ impl Imap { let (idle_wait, interrupt) = handle.wait_with_timeout(timeout); *self.interrupt.lock().await = Some(interrupt); - let res = idle_wait.await; - info!(context, "Idle finished: {:?}", res); + if *self.skip_next_idle_wait.lock().await { + // interrupt_idle has happened before we + // provided self.interrupt + *self.skip_next_idle_wait.lock().await = false; + std::mem::drop(idle_wait); + info!(context, "Idle wait was skipped"); + } else { + info!(context, "Idle entering wait-on-remote state"); + let res = idle_wait.await; + info!(context, "Idle finished wait-on-remote: {:?}", res); + } + match handle.done().await { Ok(session) => { *self.session.lock().await = Some(Session::Insecure(session)); @@ -877,7 +890,10 @@ impl Imap { pub fn interrupt_idle(&self) { task::block_on(async move { - let _ = self.interrupt.lock().await.take(); + if self.interrupt.lock().await.take().is_none() { + // idle wait is not running + *self.skip_next_idle_wait.lock().await = true; + } }); } @@ -1241,7 +1257,7 @@ impl Imap { task::block_on(async move { info!(context, "emptying folder {}", folder); - if !folder.is_empty() { + if folder.is_empty() { warn!(context, "cannot perform empty, folder not set"); return; } From ba2b66d07a8d4bc529ad5f1946de1713529d04d5 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 11 Nov 2019 15:36:14 +0100 Subject: [PATCH 3/4] fix tests for failed logins --- python/tests/test_account.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/tests/test_account.py b/python/tests/test_account.py index 750c16414..22788151e 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -896,7 +896,7 @@ class TestOnlineConfigureFails: ac1.start_threads() wait_configuration_progress(ac1, 500) ev1 = ac1._evlogger.get_matching("DC_EVENT_ERROR_NETWORK") - assert "authentication failed" in ev1[2].lower() + assert "cannot login" in ev1[2].lower() wait_configuration_progress(ac1, 0, 0) def test_invalid_user(self, acfactory): @@ -905,7 +905,7 @@ class TestOnlineConfigureFails: ac1.start_threads() wait_configuration_progress(ac1, 500) ev1 = ac1._evlogger.get_matching("DC_EVENT_ERROR_NETWORK") - assert "authentication failed" in ev1[2].lower() + assert "cannot login" in ev1[2].lower() wait_configuration_progress(ac1, 0, 0) def test_invalid_domain(self, acfactory): From 3dc589788c5f640a22d26a7d19ee55f1f794c23d Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 11 Nov 2019 15:43:21 +0100 Subject: [PATCH 4/4] actually this fixes the double import issue --- python/tests/test_account.py | 1 - 1 file changed, 1 deletion(-) diff --git a/python/tests/test_account.py b/python/tests/test_account.py index 22788151e..affe29214 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -675,7 +675,6 @@ class TestOnlineAccount: assert len(messages) == 1 assert messages[0].text == "msg1" - pytest.xfail("cannot export twice yet, probably due to interrupt_idle failing") # wait until a second passed since last backup # because get_latest_backupfile() shall return the latest backup # from a UI it's unlikely anyone manages to export two