From e388e4cc1efa77649d8a697fec3a12f00bbc90c8 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 5 Sep 2020 04:34:42 +0300 Subject: [PATCH] Do not emit network errors during configuration Previously DC_EVENT_ERROR_NETWORK events were emitted for each failed attempt during autoconfiguration, even if eventually configuration succeeded. Such error reports are not useful and often confusing, especially if they report failures to connect to domains that don't exist, such as imap.example.org when mail.example.org should be used. Now DC_EVENT_ERROR_NETWORK is only emitted when attempting to connect with existing IMAP and SMTP configuration already stored in the database. Configuration failure is still indicated by DC_EVENT_CONFIGURE_PROGRESS with data1 set to 0. Python tests from TestOnlineConfigurationFails group are changed to only wait for this event. --- python/tests/test_account.py | 9 +++---- src/imap/idle.rs | 2 +- src/imap/mod.rs | 50 ++++++++++++++++++++++++++---------- src/smtp/mod.rs | 37 ++++++++++++++------------ 4 files changed, 60 insertions(+), 38 deletions(-) diff --git a/python/tests/test_account.py b/python/tests/test_account.py index f66ceb506..e50403456 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -1898,8 +1898,7 @@ class TestOnlineConfigureFails: configtracker = ac1.configure() configtracker.wait_progress(500) configtracker.wait_progress(0) - ev = ac1._evtracker.get_matching("DC_EVENT_ERROR_NETWORK") - assert "cannot login" in ev.data2.lower() + ac1._evtracker.ensure_event_not_queued("DC_EVENT_ERROR_NETWORK") def test_invalid_user(self, acfactory): ac1, configdict = acfactory.get_online_config() @@ -1907,8 +1906,7 @@ class TestOnlineConfigureFails: configtracker = ac1.configure() configtracker.wait_progress(500) configtracker.wait_progress(0) - ev = ac1._evtracker.get_matching("DC_EVENT_ERROR_NETWORK") - assert "cannot login" in ev.data2.lower() + ac1._evtracker.ensure_event_not_queued("DC_EVENT_ERROR_NETWORK") def test_invalid_domain(self, acfactory): ac1, configdict = acfactory.get_online_config() @@ -1916,5 +1914,4 @@ class TestOnlineConfigureFails: configtracker = ac1.configure() configtracker.wait_progress(500) configtracker.wait_progress(0) - ev = ac1._evtracker.get_matching("DC_EVENT_ERROR_NETWORK") - assert "could not connect" in ev.data2.lower() + ac1._evtracker.ensure_event_not_queued("DC_EVENT_ERROR_NETWORK") diff --git a/src/imap/idle.rs b/src/imap/idle.rs index b79830268..072c14c98 100644 --- a/src/imap/idle.rs +++ b/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_if_needed(context).await?; + self.setup_handle(context).await?; self.select_folder(context, watch_folder.clone()).await?; diff --git a/src/imap/mod.rs b/src/imap/mod.rs index 6573fdb0e..33e5ed34c 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -162,7 +162,11 @@ impl Imap { self.should_reconnect = true; } - async fn setup_handle_if_needed(&mut self, context: &Context) -> Result<()> { + /// Connects or reconnects if needed. + /// + /// It is safe to call this function if already connected, actions + /// are performed only as needed. + async fn try_setup_handle(&mut self, context: &Context) -> Result<()> { if self.config.lp.server.is_empty() { bail!("IMAP operation attempted while it is torn down"); } @@ -238,9 +242,7 @@ impl Imap { ) .await }; - // IMAP connection failures are reported to users - emit_event!(context, EventType::ErrorNetwork(message)); - bail!("IMAP connection failed: {}", err); + bail!("{}: {}", message, err); } }; @@ -262,7 +264,6 @@ impl Imap { .await; warn!(context, "{} ({})", message, err); - emit_event!(context, EventType::ErrorNetwork(message.clone())); let lock = context.wrong_pw_warning_mutex.lock().await; if self.login_failed_once @@ -274,7 +275,7 @@ impl Imap { drop(lock); let mut msg = Message::new(Viewtype::Text); - msg.text = Some(message); + msg.text = Some(message.clone()); if let Err(e) = chat::add_device_msg_with_importance(context, None, Some(&mut msg), true) .await @@ -286,11 +287,24 @@ impl Imap { } self.trigger_reconnect(); - Err(format_err!("IMAP Could not login as {}", imap_user)) + Err(format_err!("{}: {}", message, err)) } } } + /// Connects or reconnects if not already connected. + /// + /// This function emits network error if it fails. It should not + /// be used during configuration to avoid showing failed attempt + /// errors to the user. + async fn setup_handle(&mut self, context: &Context) -> Result<()> { + let res = self.try_setup_handle(context).await; + if let Err(ref err) = res { + emit_event!(context, EventType::ErrorNetwork(err.to_string())); + } + res + } + async fn unsetup_handle(&mut self, context: &Context) { // Close folder if messages should be expunged if let Err(err) = self.close_folder(context).await { @@ -318,7 +332,9 @@ impl Imap { cfg.can_move = false; } - /// Connects to imap account using already-configured parameters. + /// Connects to IMAP account using already-configured parameters. + /// + /// 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(()); @@ -348,6 +364,9 @@ impl Imap { /// 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, @@ -375,8 +394,8 @@ impl Imap { config.oauth2 = oauth2; } - if let Err(err) = self.setup_handle_if_needed(context).await { - warn!(context, "failed to setup imap handle: {}", err); + if let Err(err) = self.try_setup_handle(context).await { + warn!(context, "try_setup_handle: {}", err); self.free_connect_params().await; return Err(err); } @@ -422,7 +441,10 @@ impl Imap { if teardown { self.disconnect(context).await; - bail!("IMAP disconnected immediately after connecting due to error"); + warn!( + context, + "IMAP disconnected immediately after connecting due to error" + ); } Ok(()) } @@ -437,7 +459,7 @@ impl Imap { // probably shutdown bail!("IMAP operation attempted while it is torn down"); } - self.setup_handle_if_needed(context).await?; + self.setup_handle(context).await?; while self.fetch_new_messages(context, &watch_folder).await? { // We fetch until no more new messages are there. @@ -1326,8 +1348,8 @@ impl Imap { error!(context, "cannot perform empty, folder not set"); return; } - if let Err(err) = self.setup_handle_if_needed(context).await { - error!(context, "could not setup imap connection: {:?}", err); + if let Err(_err) = self.setup_handle(context).await { + // The error is reported as a network error by setup_handle() return; } if let Err(err) = self.select_folder(context, Some(&folder)).await { diff --git a/src/smtp/mod.rs b/src/smtp/mod.rs index f69b01257..c6dbcd5e0 100644 --- a/src/smtp/mod.rs +++ b/src/smtp/mod.rs @@ -101,13 +101,26 @@ impl Smtp { } let lp = LoginParam::from_database(context, "configured_").await; - self.connect( - context, - &lp.smtp, - &lp.addr, - lp.server_flags & DC_LP_AUTH_OAUTH2 != 0, - ) - .await + let res = self + .connect( + context, + &lp.smtp, + &lp.addr, + lp.server_flags & DC_LP_AUTH_OAUTH2 != 0, + ) + .await; + if let Err(ref err) = res { + let message = context + .stock_string_repl_str2( + StockMessage::ServerResponse, + format!("SMTP {}:{}", lp.smtp.server, lp.smtp.port), + err.to_string(), + ) + .await; + + context.emit_event(EventType::ErrorNetwork(message)); + }; + res } /// Connect using the provided login params. @@ -124,7 +137,6 @@ impl Smtp { } if lp.server.is_empty() || lp.port == 0 { - context.emit_event(EventType::ErrorNetwork("SMTP bad parameters.".into())); return Err(Error::BadParameters); } @@ -197,15 +209,6 @@ impl Smtp { let mut trans = client.into_transport(); if let Err(err) = trans.connect().await { - let message = context - .stock_string_repl_str2( - StockMessage::ServerResponse, - format!("SMTP {}:{}", domain, port), - err.to_string(), - ) - .await; - - emit_event!(context, EventType::ErrorNetwork(message)); return Err(Error::ConnectionFailure(err)); }