feat: Context::set_config(): Restart IO scheduler if needed (#5111)

Restart the IO scheduler if needed to make the new config value effective (for `MvboxMove,
OnlyFetchMvbox, SentboxWatch` currently). Also add `set_config_internal()` which doesn't affect
running the IO scheduler. The reason is that `Scheduler::start()` itself calls `set_config()`,
although not for the mentioned keys, but still, and also Rust complains about recursive async calls.

Author: iequidoo

deltachat-ffi/deltachat.h

@@ -423,19 +423,16 @@ char*           dc_get_blobdir               (const dc_context_t* context);
  *                    Sending messages to self is needed for a proper multi-account setup,
  *                    however, on the other hand, may lead to unwanted notifications in non-delta clients.
  * - `sentbox_watch`= 1=watch `Sent`-folder for changes,
- *                    0=do not watch the `Sent`-folder (default),
- *                    changes require restarting IO by calling dc_stop_io() and then dc_start_io().
+ *                    0=do not watch the `Sent`-folder (default).
  * - `mvbox_move`   = 1=detect chat messages,
  *                    move them to the `DeltaChat` folder,
  *                    and watch the `DeltaChat` folder for updates (default),
  *                    0=do not move chat-messages
- *                    changes require restarting IO by calling dc_stop_io() and then dc_start_io().
  * - `only_fetch_mvbox` = 1=Do not fetch messages from folders other than the
  *                    `DeltaChat` folder. Messages will still be fetched from the
  *                    spam folder and `sendbox_watch` will also still be respected
  *                    if enabled.
  *                    0=watch all folders normally (default)
- *                    changes require restarting IO by calling dc_stop_io() and then dc_start_io().
  * - `show_emails`  = DC_SHOW_EMAILS_OFF (0)=
  *                    show direct replies to chats only,
  *                    DC_SHOW_EMAILS_ACCEPTED_CONTACTS (1)=

deltachat-jsonrpc/src/api.rs

@@ -251,7 +251,7 @@ impl CommandApi {
 
     /// Starts background tasks for a single account.
     async fn start_io(&self, account_id: u32) -> Result<()> {
-        let mut ctx = self.get_context(account_id).await?;
+        let ctx = self.get_context(account_id).await?;
         ctx.start_io().await;
         Ok(())
     }
@@ -402,7 +402,7 @@ impl CommandApi {
     /// Configures this account with the currently set parameters.
     /// Setup the credential config before calling this.
     async fn configure(&self, account_id: u32) -> Result<()> {
-        let mut ctx = self.get_context(account_id).await?;
+        let ctx = self.get_context(account_id).await?;
         ctx.stop_io().await;
         let result = ctx.configure().await;
         if result.is_err() {
@@ -2125,13 +2125,6 @@ async fn set_config(
             value,
         )
         .await?;
-
-        match key {
-            "sentbox_watch" | "mvbox_move" | "only_fetch_mvbox" => {
-                ctx.restart_io_if_running().await;
-            }
-            _ => {}
-        }
     }
     Ok(())
 }

deltachat-repl/src/main.rs

@@ -401,7 +401,7 @@ enum ExitResult {
 
 async fn handle_cmd(
     line: &str,
-    mut ctx: Context,
+    ctx: Context,
     selected_chat: &mut ChatId,
 ) -> Result<ExitResult, Error> {
     let mut args = line.splitn(2, ' ');

python/tests/test_1_online.py

@@ -382,6 +382,21 @@ def test_webxdc_download_on_demand(acfactory, data, lp):
     assert msgs_changed_event.data2 == 0
 
 
+def test_enable_mvbox_move(acfactory, lp):
+    (ac1,) = acfactory.get_online_accounts(1)
+
+    lp.sec("ac2: start without mvbox thread")
+    ac2 = acfactory.new_online_configuring_account(mvbox_move=False)
+    acfactory.bring_accounts_online()
+
+    lp.sec("ac2: configuring mvbox")
+    ac2.set_config("mvbox_move", "1")
+
+    lp.sec("ac1: send message and wait for ac2 to receive it")
+    acfactory.get_accepted_chat(ac1, ac2).send_text("message1")
+    assert ac2._evtracker.wait_next_incoming_message().text == "message1"
+
+
 def test_mvbox_sentbox_threads(acfactory, lp):
     lp.sec("ac1: start with mvbox thread")
     ac1 = acfactory.new_online_configuring_account(mvbox_move=True, sentbox_watch=True)

src/authres.rs

@@ -216,7 +216,7 @@ async fn update_authservid_candidates(
     if old_ids != new_ids {
         let new_config = new_ids.into_iter().collect::<Vec<_>>().join(" ");
         context
-            .set_config(Config::AuthservIdCandidates, Some(&new_config))
+            .set_config_internal(Config::AuthservIdCandidates, Some(&new_config))
             .await?;
         // Updating the authservid candidates may mean that we now consider
         // emails as "failed" which "passed" previously, so we need to

src/chat.rs

@@ -783,7 +783,9 @@ impl ChatId {
 
         context.emit_msgs_changed_without_ids();
 
-        context.set_config(Config::LastHousekeeping, None).await?;
+        context
+            .set_config_internal(Config::LastHousekeeping, None)
+            .await?;
         context.scheduler.interrupt_inbox().await;
 
         if chat.is_self_talk() {
@@ -4266,7 +4268,9 @@ pub(crate) async fn delete_and_reset_all_device_msgs(context: &Context) -> Resul
             (),
         )
         .await?;
-    context.set_config(Config::QuotaExceeding, None).await?;
+    context
+        .set_config_internal(Config::QuotaExceeding, None)
+        .await?;
     Ok(())
 }
 

src/config.rs

@@ -367,11 +367,23 @@ impl Config {
     /// multiple users are sharing an account. Another example is `Self::SyncMsgs` itself which
     /// mustn't be controlled by other devices.
     pub(crate) fn is_synced(&self) -> bool {
+        // We don't restart IO from the synchronisation code, so this is to be on the safe side.
+        if self.needs_io_restart() {
+            return false;
+        }
         matches!(
             self,
             Self::Displayname | Self::MdnsEnabled | Self::ShowEmails
         )
     }
+
+    /// Whether the config option needs an IO scheduler restart to take effect.
+    pub(crate) fn needs_io_restart(&self) -> bool {
+        matches!(
+            self,
+            Config::MvboxMove | Config::OnlyFetchMvbox | Config::SentboxWatch
+        )
+    }
 }
 
 impl Context {
@@ -491,10 +503,50 @@ impl Context {
         }
     }
 
-    /// Set the given config key.
-    /// If `None` is passed as a value the value is cleared and set to the default if there is one.
+    fn check_config(key: Config, value: Option<&str>) -> Result<()> {
+        match key {
+            Config::Socks5Enabled
+            | Config::BccSelf
+            | Config::E2eeEnabled
+            | Config::MdnsEnabled
+            | Config::SentboxWatch
+            | Config::MvboxMove
+            | Config::OnlyFetchMvbox
+            | Config::FetchExistingMsgs
+            | Config::DeleteToTrash
+            | Config::SaveMimeHeaders
+            | Config::Configured
+            | Config::Bot
+            | Config::NotifyAboutWrongPw
+            | Config::SyncMsgs
+            | Config::SignUnencrypted
+            | Config::DisableIdle => {
+                ensure!(
+                    matches!(value, None | Some("0") | Some("1")),
+                    "Boolean value must be either 0 or 1"
+                );
+            }
+            _ => (),
+        }
+        Ok(())
+    }
+
+    /// Set the given config key and make it effective.
+    /// This may restart the IO scheduler. If `None` is passed as a value the value is cleared and
+    /// set to the default if there is one.
     pub async fn set_config(&self, key: Config, value: Option<&str>) -> Result<()> {
-        self.set_config_ex(key.is_synced().into(), key, value).await
+        Self::check_config(key, value)?;
+
+        let _pause = match key.needs_io_restart() {
+            true => self.scheduler.pause(self.clone()).await?,
+            _ => Default::default(),
+        };
+        self.set_config_internal(key, value).await?;
+        Ok(())
+    }
+
+    pub(crate) async fn set_config_internal(&self, key: Config, value: Option<&str>) -> Result<()> {
+        self.set_config_ex(Sync, key, value).await
     }
 
     pub(crate) async fn set_config_ex(
@@ -503,7 +555,10 @@ impl Context {
         key: Config,
         mut value: Option<&str>,
     ) -> Result<()> {
+        Self::check_config(key, value)?;
+        let sync = sync == Sync && key.is_synced();
         let better_value;
+
         match key {
             Config::Selfavatar => {
                 self.sql
@@ -536,28 +591,6 @@ impl Context {
                 }
                 self.sql.set_raw_config(key.as_ref(), value).await?;
             }
-            Config::Socks5Enabled
-            | Config::BccSelf
-            | Config::E2eeEnabled
-            | Config::MdnsEnabled
-            | Config::SentboxWatch
-            | Config::MvboxMove
-            | Config::OnlyFetchMvbox
-            | Config::FetchExistingMsgs
-            | Config::DeleteToTrash
-            | Config::SaveMimeHeaders
-            | Config::Configured
-            | Config::Bot
-            | Config::NotifyAboutWrongPw
-            | Config::SyncMsgs
-            | Config::SignUnencrypted
-            | Config::DisableIdle => {
-                ensure!(
-                    matches!(value, None | Some("0") | Some("1")),
-                    "Boolean value must be either 0 or 1"
-                );
-                self.sql.set_raw_config(key.as_ref(), value).await?;
-            }
             Config::Addr => {
                 self.sql
                     .set_raw_config(key.as_ref(), value.map(|s| s.to_lowercase()).as_deref())
@@ -570,7 +603,7 @@ impl Context {
         if key.is_synced() {
             self.emit_event(EventType::ConfigSynced { key });
         }
-        if sync != Sync {
+        if !sync {
             return Ok(());
         }
         let Some(val) = value else {
@@ -597,8 +630,7 @@ impl Context {
 
     /// Set the given config to a boolean value.
     pub async fn set_config_bool(&self, key: Config, value: bool) -> Result<()> {
-        self.set_config(key, if value { Some("1") } else { Some("0") })
-            .await?;
+        self.set_config(key, from_bool(value)).await?;
         Ok(())
     }
 
@@ -618,6 +650,11 @@ impl Context {
     }
 }
 
+/// Returns a value for use in `Context::set_config_*()` for the given `bool`.
+pub(crate) fn from_bool(val: bool) -> Option<&'static str> {
+    Some(if val { "1" } else { "0" })
+}
+
 // Separate impl block for self address handling
 impl Context {
     /// Determine whether the specified addr maps to the/a self addr.
@@ -644,13 +681,13 @@ impl Context {
         let mut secondary_addrs = self.get_all_self_addrs().await?;
         // never store a primary address also as a secondary
         secondary_addrs.retain(|a| !addr_cmp(a, primary_new));
-        self.set_config(
+        self.set_config_internal(
             Config::SecondaryAddrs,
             Some(secondary_addrs.join(" ").as_str()),
         )
         .await?;
 
-        self.set_config(Config::ConfiguredAddr, Some(primary_new))
+        self.set_config_internal(Config::ConfiguredAddr, Some(primary_new))
             .await?;
 
         Ok(())

src/configure.rs

@@ -22,7 +22,7 @@ use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC};
 use server_params::{expand_param_vector, ServerParams};
 use tokio::task;
 
-use crate::config::Config;
+use crate::config::{self, Config};
 use crate::contact::addr_cmp;
 use crate::context::Context;
 use crate::imap::Imap;
@@ -112,12 +112,13 @@ impl Context {
         let mut param = LoginParam::load_candidate_params(self).await?;
         let old_addr = self.get_config(Config::ConfiguredAddr).await?;
         let success = configure(self, &mut param).await;
-        self.set_config(Config::NotifyAboutWrongPw, None).await?;
+        self.set_config_internal(Config::NotifyAboutWrongPw, None)
+            .await?;
 
         on_configure_completed(self, param, old_addr).await?;
 
         success?;
-        self.set_config(Config::NotifyAboutWrongPw, Some("1"))
+        self.set_config_internal(Config::NotifyAboutWrongPw, Some("1"))
             .await?;
         Ok(())
     }
@@ -473,15 +474,15 @@ async fn configure(ctx: &Context, param: &mut LoginParam) -> Result<()> {
 
     // the trailing underscore is correct
     param.save_as_configured_params(ctx).await?;
-    ctx.set_config(Config::ConfiguredTimestamp, Some(&time().to_string()))
+    ctx.set_config_internal(Config::ConfiguredTimestamp, Some(&time().to_string()))
         .await?;
 
     progress!(ctx, 920);
 
     e2ee::ensure_secret_key_exists(ctx).await?;
     info!(ctx, "key generation completed");
 
-    ctx.set_config_bool(Config::FetchedExistingMsgs, false)
+    ctx.set_config_internal(Config::FetchedExistingMsgs, config::from_bool(false))
         .await?;
     ctx.scheduler.interrupt_inbox().await;
 

src/contact.rs

@@ -1550,7 +1550,7 @@ pub(crate) async fn set_profile_image(
             if contact_id == ContactId::SELF {
                 if was_encrypted {
                     context
-                        .set_config(Config::Selfavatar, Some(profile_image))
+                        .set_config_internal(Config::Selfavatar, Some(profile_image))
                         .await?;
                 } else {
                     info!(context, "Do not use unencrypted selfavatar.");
@@ -1563,7 +1563,9 @@ pub(crate) async fn set_profile_image(
         AvatarAction::Delete => {
             if contact_id == ContactId::SELF {
                 if was_encrypted {
-                    context.set_config(Config::Selfavatar, None).await?;
+                    context
+                        .set_config_internal(Config::Selfavatar, None)
+                        .await?;
                 } else {
                     info!(context, "Do not use unencrypted selfavatar deletion.");
                 }
@@ -1595,7 +1597,7 @@ pub(crate) async fn set_status(
     if contact_id == ContactId::SELF {
         if encrypted && has_chat_version {
             context
-                .set_config(Config::Selfstatus, Some(&status))
+                .set_config_internal(Config::Selfstatus, Some(&status))
                 .await?;
         }
     } else {

src/context.rs

@@ -408,7 +408,7 @@ impl Context {
     }
 
     /// Starts the IO scheduler.
-    pub async fn start_io(&mut self) {
+    pub async fn start_io(&self) {
         if !self.is_configured().await.unwrap_or_default() {
             warn!(self, "can not start io on a context that is not configured");
             return;