imap: refactor to always create Imap configured

`Imap` structure is always created in a configured state now. There is
no default value for `ImapConfig` anymore.

Also resultify Scheduler::start() to fail on database errors, for
example if IMAP configuration cannot be read from the database during
`start_io()`. Previosuly errors during reading keys such as
`mvbox_watch` were simply ignored and folders were not watched until
the application is completely restarted, now start_io() will fail and
scheduler will only be started at the next start_io() call which
usually happens when app is brought to the foreground.

src/configure.rs

@@ -345,10 +345,8 @@ async fn configure(ctx: &Context, param: &mut LoginParam) -> Result<()> {
     progress!(ctx, 600);
 
     // Configure IMAP
-    let (_s, r) = async_std::channel::bounded(1);
-    let mut imap = Imap::new(r);
 
-    let mut imap_configured = false;
+    let mut imap: Option<Imap> = None;
     let imap_servers: Vec<&ServerParams> = servers
         .iter()
         .filter(|params| params.protocol == Protocol::Imap)
@@ -361,18 +359,9 @@ async fn configure(ctx: &Context, param: &mut LoginParam) -> Result<()> {
         param.imap.port = imap_server.port;
         param.imap.security = imap_server.socket;
 
-        match try_imap_one_param(
+        match try_imap_one_param(ctx, &param.imap, &param.addr, oauth2, provider_strict_tls).await {
-            ctx,
+            Ok(configured_imap) => {
-            &param.imap,
+                imap = Some(configured_imap);
-            &param.addr,
-            oauth2,
-            provider_strict_tls,
-            &mut imap,
-        )
-        .await
-        {
-            Ok(_) => {
-                imap_configured = true;
                 break;
             }
             Err(e) => errors.push(e),
@@ -382,9 +371,10 @@ async fn configure(ctx: &Context, param: &mut LoginParam) -> Result<()> {
             600 + (800 - 600) * (1 + imap_server_index) / imap_servers_count
         );
     }
-    if !imap_configured {
+    let mut imap = match imap {
-        bail!(nicer_configuration_error(ctx, errors).await);
+        Some(imap) => imap,
-    }
+        None => bail!(nicer_configuration_error(ctx, errors).await),
+    };
 
     progress!(ctx, 850);
 
@@ -520,26 +510,38 @@ async fn try_imap_one_param(
     addr: &str,
     oauth2: bool,
     provider_strict_tls: bool,
-    imap: &mut Imap,
+) -> Result<Imap, ConfigurationError> {
-) -> Result<(), ConfigurationError> {
     let inf = format!(
         "imap: {}@{}:{} security={} certificate_checks={} oauth2={}",
         param.user, param.server, param.port, param.security, param.certificate_checks, oauth2
     );
     info!(context, "Trying: {}", inf);
 
-    if let Err(err) = imap
+    let (_s, r) = async_std::channel::bounded(1);
-        .connect(context, param, addr, oauth2, provider_strict_tls)
+
-        .await
+    let mut imap = match Imap::new(param, addr, oauth2, provider_strict_tls, r).await {
-    {
+        Err(err) => {
             info!(context, "failure: {}", err);
-        Err(ConfigurationError {
+            return Err(ConfigurationError {
                 config: inf,
                 msg: err.to_string(),
-        })
+            });
-    } else {
+        }
+        Ok(imap) => imap,
+    };
+
+    match imap.connect(context).await {
+        Err(err) => {
+            info!(context, "failure: {}", err);
+            return Err(ConfigurationError {
+                config: inf,
+                msg: err.to_string(),
+            });
+        }
+        Ok(()) => {
             info!(context, "success: {}", inf);
-        Ok(())
+            return Ok(imap);
+        }
     }
 }
 

src/context.rs

@@ -161,7 +161,9 @@ impl Context {
 
         {
             let l = &mut *self.inner.scheduler.write().await;
-            l.start(self.clone()).await;
+            if let Err(err) = l.start(self.clone()).await {
+                error!(self, "Failed to start IO: {}", err)
+            }
         }
     }
 

src/imap.rs

@@ -8,7 +8,7 @@ use std::{cmp, cmp::max, collections::BTreeMap};
 use anyhow::{bail, format_err, Context as _, Result};
 use async_imap::{
     error::Result as ImapResult,
-    types::{Capability, Fetch, Flag, Mailbox, Name, NameAttribute},
+    types::{Fetch, Flag, Mailbox, Name, NameAttribute},
 };
 use async_std::channel::Receiver;
 use async_std::prelude::*;
@@ -92,6 +92,10 @@ pub struct Imap {
     interrupt: Option<stop_token::StopSource>,
     should_reconnect: bool,
     login_failed_once: bool,
+
+    /// True if CAPABILITY command was run successfully once and config.can_* contain correct
+    /// values.
+    capabilities_determined: bool,
 }
 
 #[derive(Debug)]
@@ -141,6 +145,7 @@ struct ImapConfig {
     pub selected_folder: Option<String>,
     pub selected_mailbox: Option<Mailbox>,
     pub selected_folder_needs_expunge: bool,
+
     pub can_idle: bool,
 
     /// True if the server has MOVE capability as defined in
@@ -148,58 +153,93 @@ struct ImapConfig {
     pub can_move: bool,
 }
 
-impl Default for ImapConfig {
+impl Imap {
-    fn default() -> Self {
+    /// Creates new disconnected IMAP client using the specific login parameters.
-        ImapConfig {
+    ///
-            addr: "".into(),
+    /// `addr` is used to renew token if OAuth2 authentication is used.
-            lp: Default::default(),
+    pub async fn new(
-            strict_tls: false,
+        lp: &ServerLoginParam,
-            oauth2: false,
+        addr: &str,
+        oauth2: bool,
+        provider_strict_tls: bool,
+        idle_interrupt: Receiver<InterruptInfo>,
+    ) -> Result<Self> {
+        if lp.server.is_empty() || lp.user.is_empty() || lp.password.is_empty() {
+            bail!("Incomplete IMAP connection parameters");
+        }
+
+        let strict_tls = match lp.certificate_checks {
+            CertificateChecks::Automatic => provider_strict_tls,
+            CertificateChecks::Strict => true,
+            CertificateChecks::AcceptInvalidCertificates
+            | CertificateChecks::AcceptInvalidCertificates2 => false,
+        };
+        let config = ImapConfig {
+            addr: addr.to_string(),
+            lp: lp.clone(),
+            strict_tls,
+            oauth2,
             selected_folder: None,
             selected_mailbox: None,
             selected_folder_needs_expunge: false,
             can_idle: false,
             can_move: false,
-        }
+        };
-    }
-}
 
-impl Imap {
+        let imap = Imap {
-    pub fn new(idle_interrupt: Receiver<InterruptInfo>) -> Self {
-        Imap {
             idle_interrupt,
-            config: Default::default(),
+            config,
-            session: Default::default(),
+            session: None,
-            connected: Default::default(),
+            connected: false,
-            interrupt: Default::default(),
+            interrupt: None,
-            should_reconnect: Default::default(),
+            should_reconnect: false,
-            login_failed_once: Default::default(),
+            login_failed_once: false,
-        }
+            capabilities_determined: false,
+        };
+
+        Ok(imap)
     }
 
-    pub fn is_connected(&self) -> bool {
+    /// Creates new disconnected IMAP client using configured parameters.
-        self.connected
+    pub async fn new_configured(
+        context: &Context,
+        idle_interrupt: Receiver<InterruptInfo>,
+    ) -> Result<Self> {
+        if !context.is_configured().await? {
+            bail!("IMAP Connect without configured params");
         }
 
-    pub fn should_reconnect(&self) -> bool {
+        let param = LoginParam::from_database(context, "configured_").await?;
-        self.should_reconnect
+        // the trailing underscore is correct
-    }
 
-    pub fn trigger_reconnect(&mut self) {
+        let imap = Self::new(
-        self.should_reconnect = true;
+            &param.imap,
+            &param.addr,
+            param.server_flags & DC_LP_AUTH_OAUTH2 != 0,
+            param.provider.map_or(false, |provider| provider.strict_tls),
+            idle_interrupt,
+        )
+        .await?;
+        Ok(imap)
     }
 
     /// Connects or reconnects if needed.
     ///
-    /// It is safe to call this function if already connected, actions
+    /// It is safe to call this function if already connected, actions are performed only as needed.
-    /// are performed only as needed.
+    ///
-    async fn try_setup_handle(&mut self, context: &Context) -> Result<()> {
+    /// Does not emit network errors, can be used to try various parameters during
+    /// autoconfiguration.
+    ///
+    /// Calling this function is not enough to perform IMAP operations. Use [`Imap::prepare`]
+    /// instead if you are going to actually use connection rather than trying connection
+    /// parameters.
+    pub async fn connect(&mut self, context: &Context) -> Result<()> {
         if self.config.lp.server.is_empty() {
             bail!("IMAP operation attempted while it is torn down");
         }
 
         if self.should_reconnect() {
-            self.unsetup_handle(context).await;
+            self.disconnect(context).await;
             self.should_reconnect = false;
         } else if self.is_connected() {
             return Ok(());
@@ -269,6 +309,10 @@ impl Imap {
                 self.connected = true;
                 self.session = Some(session);
                 self.login_failed_once = false;
+                emit_event!(
+                    context,
+                    EventType::ImapConnected(format!("IMAP-LOGIN as {}", self.config.lp.user))
+                );
                 Ok(())
             }
 
@@ -305,20 +349,49 @@ impl Imap {
         }
     }
 
-    /// Connects or reconnects if not already connected.
+    /// Determine server capabilities if not done yet.
+    async fn determine_capabilities(&mut self) -> Result<()> {
+        if self.capabilities_determined {
+            return Ok(());
+        }
+
+        match &mut self.session {
+            Some(ref mut session) => match session.capabilities().await {
+                Ok(caps) => {
+                    self.config.can_idle = caps.has_str("IDLE");
+                    self.config.can_move = caps.has_str("MOVE");
+                    self.capabilities_determined = true;
+                    Ok(())
+                }
+                Err(err) => {
+                    bail!("CAPABILITY command error: {}", err);
+                }
+            },
+            None => {
+                bail!("Can't determine server capabilities because connection was not established")
+            }
+        }
+    }
+
+    /// Prepare for IMAP operation.
     ///
-    /// This function emits network error if it fails.  It should not
+    /// Ensure that IMAP client is connected, folders are created and IMAP capabilities are
-    /// be used during configuration to avoid showing failed attempt
+    /// determined.
-    /// errors to the user.
+    ///
-    async fn setup_handle(&mut self, context: &Context) -> Result<()> {
+    /// This function emits network error if it fails.  It should not be used during configuration
-        let res = self.try_setup_handle(context).await;
+    /// to avoid showing failed attempt errors to the user.
+    pub async fn prepare(&mut self, context: &Context) -> Result<()> {
+        let res = self.connect(context).await;
         if let Err(ref err) = res {
             emit_event!(context, EventType::ErrorNetwork(err.to_string()));
         }
+
+        self.ensure_configured_folders(context, true).await?;
+        self.determine_capabilities().await?;
         res
     }
 
-    async fn unsetup_handle(&mut self, context: &Context) {
+    async fn disconnect(&mut self, context: &Context) {
         // Close folder if messages should be expunged
         if let Err(err) = self.close_folder(context).await {
             warn!(context, "failed to close folder: {:?}", err);
@@ -331,139 +404,21 @@ impl Imap {
             }
         }
         self.connected = false;
+        self.capabilities_determined = false;
         self.config.selected_folder = None;
         self.config.selected_mailbox = None;
     }
 
-    async fn free_connect_params(&mut self) {
+    pub fn is_connected(&self) -> bool {
-        let mut cfg = &mut self.config;
+        self.connected
-
-        cfg.addr = "".into();
-        cfg.lp = Default::default();
-
-        cfg.can_idle = false;
-        cfg.can_move = false;
     }
 
-    /// Connects to IMAP account using already-configured parameters.
+    pub fn should_reconnect(&self) -> bool {
-    ///
+        self.should_reconnect
-    /// Emits network error if connection fails.
-    pub async fn connect_configured(&mut self, context: &Context) -> Result<()> {
-        if self.is_connected() && !self.should_reconnect() {
-            return Ok(());
-        }
-        if !context.is_configured().await? {
-            bail!("IMAP Connect without configured params");
     }
 
-        let param = LoginParam::from_database(context, "configured_").await?;
+    pub fn trigger_reconnect(&mut self) {
-        // the trailing underscore is correct
+        self.should_reconnect = true;
-
-        if let Err(err) = self
-            .connect(
-                context,
-                &param.imap,
-                &param.addr,
-                param.server_flags & DC_LP_AUTH_OAUTH2 != 0,
-                param.provider.map_or(false, |provider| provider.strict_tls),
-            )
-            .await
-        {
-            bail!("IMAP Connection Failed with params {}: {}", param, err);
-        } else {
-            self.ensure_configured_folders(context, true).await
-        }
-    }
-
-    /// Tries connecting to imap account using the specific login parameters.
-    ///
-    /// `addr` is used to renew token if OAuth2 authentication is used.
-    ///
-    /// Does not emit network errors, can be used to try various
-    /// parameters during autoconfiguration.
-    pub async fn connect(
-        &mut self,
-        context: &Context,
-        lp: &ServerLoginParam,
-        addr: &str,
-        oauth2: bool,
-        provider_strict_tls: bool,
-    ) -> Result<()> {
-        if lp.server.is_empty() || lp.user.is_empty() || lp.password.is_empty() {
-            bail!("Incomplete IMAP connection parameters");
-        }
-
-        {
-            let mut config = &mut self.config;
-            config.addr = addr.to_string();
-            config.lp = lp.clone();
-            config.strict_tls = match lp.certificate_checks {
-                CertificateChecks::Automatic => provider_strict_tls,
-                CertificateChecks::Strict => true,
-                CertificateChecks::AcceptInvalidCertificates
-                | CertificateChecks::AcceptInvalidCertificates2 => false,
-            };
-            config.oauth2 = oauth2;
-        }
-
-        if let Err(err) = self.try_setup_handle(context).await {
-            warn!(context, "try_setup_handle: {}", err);
-            self.free_connect_params().await;
-            return Err(err);
-        }
-
-        let teardown = match &mut self.session {
-            Some(ref mut session) => match session.capabilities().await {
-                Ok(caps) => {
-                    if !context.sql.is_open().await {
-                        warn!(context, "IMAP-LOGIN as {} ok but ABORTING", lp.user,);
-                        true
-                    } else {
-                        let can_idle = caps.has_str("IDLE");
-                        let can_move = caps.has_str("MOVE");
-                        let caps_list = caps.iter().fold(String::new(), |s, c| {
-                            if let Capability::Atom(x) = c {
-                                s + &format!(" {}", x)
-                            } else {
-                                s + &format!(" {:?}", c)
-                            }
-                        });
-
-                        self.config.can_idle = can_idle;
-                        self.config.can_move = can_move;
-                        self.connected = true;
-                        emit_event!(
-                            context,
-                            EventType::ImapConnected(format!(
-                                "IMAP-LOGIN as {}, capabilities: {}",
-                                lp.user, caps_list,
-                            ))
-                        );
-                        false
-                    }
-                }
-                Err(err) => {
-                    info!(context, "CAPABILITY command error: {}", err);
-                    true
-                }
-            },
-            None => true,
-        };
-
-        if teardown {
-            self.disconnect(context).await;
-
-            warn!(
-                context,
-                "IMAP disconnected immediately after connecting due to error"
-            );
-        }
-        Ok(())
-    }
-
-    pub async fn disconnect(&mut self, context: &Context) {
-        self.unsetup_handle(context).await;
-        self.free_connect_params().await;
     }
 
     pub async fn fetch(&mut self, context: &Context, watch_folder: &str) -> Result<()> {
@@ -471,7 +426,7 @@ impl Imap {
             // probably shutdown
             bail!("IMAP operation attempted while it is torn down");
         }
-        self.setup_handle(context).await?;
+        self.prepare(context).await?;
 
         while self
             .fetch_new_messages(context, &watch_folder, false)
@@ -988,10 +943,6 @@ impl Imap {
         (last_uid, read_errors)
     }
 
-    pub async fn can_move(&self) -> bool {
-        self.config.can_move
-    }
-
     pub async fn mv(
         &mut self,
         context: &Context,
@@ -1016,7 +967,7 @@ impl Imap {
         let set = format!("{}", uid);
         let display_folder_id = format!("{}/{}", folder, uid);
 
-        if self.can_move().await {
+        if self.config.can_move {
             if let Some(ref mut session) = &mut self.session {
                 match session.uid_mv(&set, &dest_folder).await {
                     Ok(_) => {
@@ -1142,7 +1093,7 @@ impl Imap {
             // TODO: make INBOX/SENT/MVBOX perform the jobs on their
             // respective folders to avoid select_folder network traffic
             // and the involved error states
-            if let Err(err) = self.connect_configured(context).await {
+            if let Err(err) = self.prepare(context).await {
                 warn!(context, "prepare_imap_op failed: {}", err);
                 return Some(ImapActionResult::RetryLater);
             }

src/imap/idle.rs

@@ -25,7 +25,7 @@ impl Imap {
         if !self.can_idle() {
             bail!("IMAP server does not have IDLE capability");
         }
-        self.setup_handle(context).await?;
+        self.prepare(context).await?;
 
         self.select_folder(context, watch_folder.as_deref()).await?;
 
@@ -158,7 +158,7 @@ impl Imap {
                     // try to connect with proper login params
                     // (setup_handle_if_needed might not know about them if we
                     // never successfully connected)
-                    if let Err(err) = self.connect_configured(context).await {
+                    if let Err(err) = self.prepare(context).await {
                         warn!(context, "fake_idle: could not connect: {}", err);
                         continue;
                     }

src/imap/scan_folders.rs

@@ -25,7 +25,7 @@ impl Imap {
         }
         info!(context, "Starting full folder scan");
 
-        self.connect_configured(context).await?;
+        self.prepare(context).await?;
         let session = self.session.as_mut();
         let session = session.context("scan_folders(): IMAP No Connection established")?;
         let folders: Vec<_> = session.list(Some(""), Some("*")).await?.collect().await;

src/job.rs

@@ -532,7 +532,7 @@ impl Job {
     }
 
     async fn move_msg(&mut self, context: &Context, imap: &mut Imap) -> Status {
-        if let Err(err) = imap.connect_configured(context).await {
+        if let Err(err) = imap.prepare(context).await {
             warn!(context, "could not connect: {:?}", err);
             return Status::RetryLater;
         }
@@ -594,7 +594,7 @@ impl Job {
     /// records pointing to the same message on the server, the job
     /// also removes the message on the server.
     async fn delete_msg_on_imap(&mut self, context: &Context, imap: &mut Imap) -> Status {
-        if let Err(err) = imap.connect_configured(context).await {
+        if let Err(err) = imap.prepare(context).await {
             warn!(context, "could not connect: {:?}", err);
             return Status::RetryLater;
         }
@@ -682,7 +682,7 @@ impl Job {
         if job_try!(context.get_config_bool(Config::Bot).await) {
             return Status::Finished(Ok(())); // Bots don't want those messages
         }
-        if let Err(err) = imap.connect_configured(context).await {
+        if let Err(err) = imap.prepare(context).await {
             warn!(context, "could not connect: {:?}", err);
             return Status::RetryLater;
         }
@@ -755,7 +755,7 @@ impl Job {
     /// Chat in contrast to the Sent folder, which is normally managed
     /// by the user via webmail or another email client.
     async fn resync_folders(&mut self, context: &Context, imap: &mut Imap) -> Status {
-        if let Err(err) = imap.connect_configured(context).await {
+        if let Err(err) = imap.prepare(context).await {
             warn!(context, "could not connect: {:?}", err);
             return Status::RetryLater;
         }
@@ -779,7 +779,7 @@ impl Job {
     }
 
     async fn markseen_msg_on_imap(&mut self, context: &Context, imap: &mut Imap) -> Status {
-        if let Err(err) = imap.connect_configured(context).await {
+        if let Err(err) = imap.prepare(context).await {
             warn!(context, "could not connect: {:?}", err);
             return Status::RetryLater;
         }

src/scheduler.rs

@@ -1,3 +1,4 @@
+use anyhow::{bail, Result};
 use async_std::prelude::*;
 use async_std::{
     channel::{self, Receiver, Sender},
@@ -130,7 +131,7 @@ async fn inbox_loop(ctx: Context, started: Sender<()>, inbox_handlers: ImapConne
 async fn fetch(ctx: &Context, connection: &mut Imap) {
     match ctx.get_config(Config::ConfiguredInboxFolder).await {
         Ok(Some(watch_folder)) => {
-            if let Err(err) = connection.connect_configured(ctx).await {
+            if let Err(err) = connection.prepare(ctx).await {
                 error_network!(ctx, "{}", err);
                 return;
             }
@@ -159,7 +160,7 @@ async fn fetch_idle(ctx: &Context, connection: &mut Imap, folder: Config) -> Int
     match ctx.get_config(folder).await {
         Ok(Some(watch_folder)) => {
             // connect and fake idle if unable to connect
-            if let Err(err) = connection.connect_configured(ctx).await {
+            if let Err(err) = connection.prepare(ctx).await {
                 warn!(ctx, "imap connection failed: {}", err);
                 return connection.fake_idle(ctx, Some(watch_folder)).await;
             }
@@ -301,11 +302,11 @@ async fn smtp_loop(ctx: Context, started: Sender<()>, smtp_handlers: SmtpConnect
 
 impl Scheduler {
     /// Start the scheduler, panics if it is already running.
-    pub async fn start(&mut self, ctx: Context) {
+    pub async fn start(&mut self, ctx: Context) -> Result<()> {
-        let (mvbox, mvbox_handlers) = ImapConnectionState::new();
+        let (mvbox, mvbox_handlers) = ImapConnectionState::new(&ctx).await?;
-        let (sentbox, sentbox_handlers) = ImapConnectionState::new();
+        let (sentbox, sentbox_handlers) = ImapConnectionState::new(&ctx).await?;
         let (smtp, smtp_handlers) = SmtpConnectionState::new();
-        let (inbox, inbox_handlers) = ImapConnectionState::new();
+        let (inbox, inbox_handlers) = ImapConnectionState::new(&ctx).await?;
 
         let (inbox_start_send, inbox_start_recv) = channel::bounded(1);
         let (mvbox_start_send, mvbox_start_recv) = channel::bounded(1);
@@ -321,11 +322,7 @@ impl Scheduler {
             }))
         };
 
-        if ctx
+        if ctx.get_config_bool(Config::MvboxWatch).await? {
-            .get_config_bool(Config::MvboxWatch)
-            .await
-            .unwrap_or_default()
-        {
             let ctx = ctx.clone();
             mvbox_handle = Some(task::spawn(async move {
                 simple_imap_loop(
@@ -343,11 +340,7 @@ impl Scheduler {
                 .expect("mvbox start send, missing receiver");
         }
 
-        if ctx
+        if ctx.get_config_bool(Config::SentboxWatch).await? {
-            .get_config_bool(Config::SentboxWatch)
-            .await
-            .unwrap_or_default()
-        {
             let ctx = ctx.clone();
             sentbox_handle = Some(task::spawn(async move {
                 simple_imap_loop(
@@ -391,10 +384,11 @@ impl Scheduler {
             .try_join(smtp_start_recv.recv())
             .await
         {
-            error!(ctx, "failed to start scheduler: {}", err);
+            bail!("failed to start scheduler: {}", err);
         }
 
         info!(ctx, "scheduler is running");
+        Ok(())
     }
 
     async fn maybe_network(&self) {
@@ -588,13 +582,13 @@ pub(crate) struct ImapConnectionState {
 
 impl ImapConnectionState {
     /// Construct a new connection.
-    fn new() -> (Self, ImapConnectionHandlers) {
+    async fn new(context: &Context) -> Result<(Self, ImapConnectionHandlers)> {
         let (stop_sender, stop_receiver) = channel::bounded(1);
         let (shutdown_sender, shutdown_receiver) = channel::bounded(1);
         let (idle_interrupt_sender, idle_interrupt_receiver) = channel::bounded(1);
 
         let handlers = ImapConnectionHandlers {
-            connection: Imap::new(idle_interrupt_receiver),
+            connection: Imap::new_configured(context, idle_interrupt_receiver).await?,
             stop_receiver,
             shutdown_sender,
         };
@@ -607,7 +601,7 @@ impl ImapConnectionState {
 
         let conn = ImapConnectionState { state };
 
-        (conn, handlers)
+        Ok((conn, handlers))
     }
 
     /// Interrupt any form of idle.