Improve backoff and behaviour in case of RetryAfter errors

2024-10-23 17:36:54 +02:00 · 2024-07-27 23:23:12 +03:00 · 2024-07-27 23:23:12 +03:00 · 3da5a1a4c5
commit 3da5a1a4c5
parent ccfc6165e6
9 changed files with 136 additions and 15 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -67,6 +67,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Feature `sqlite-storage` was renamed to `sqlite-storage-nativetls`([PR 995](https://github.com/teloxide/teloxide/pull/995))
 - MSRV (Minimal Supported Rust Version) was bumped from `1.68.0` to `1.70.0` ([PR 996][https://github.com/teloxide/teloxide/pull/996])
 - `axum` was bumped to `0.7`, along with related libraries used for webhooks ([PR 1093][https://github.com/teloxide/teloxide/pull/1093])
+- Exponential backoff now results in 64 seconds maximum delay instead of 17 minutes ([PR 1112][https://github.com/teloxide/teloxide/pull/1112])

 ### Removed

--- a/Cargo.lock
+++ b/Cargo.lock
@ -691,6 +691,12 @@ version = "0.3.30"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "38d84fa142264698cdce1a9f9172cf383a0c82de1bddcf3092901442c4097004"

+[[package]]
+name = "futures-timer"
+version = "3.0.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f288b0a4f20f9a56b5d1da57e2227c661b7b16168e2f72365f57b63326e29b24"
+
 [[package]]
 name = "futures-util"
 version = "0.3.30"
@ -736,6 +742,12 @@ version = "0.29.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "40ecd4077b5ae9fd2e9e169b102c6c330d0605168eb0e8bf79952b256dbefffd"

+[[package]]
+name = "glob"
+version = "0.3.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b"
+
 [[package]]
 name = "h2"
 version = "0.3.26"
@ -1444,6 +1456,15 @@ dependencies = [
 "log",
 ]

+[[package]]
+name = "proc-macro-crate"
+version = "3.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "6d37c51ca738a55da99dc0c4a34860fd675453b8b36209178c2249bb13651284"
+dependencies = [
+ "toml_edit",
+]
+
 [[package]]
 name = "proc-macro-error"
 version = "1.0.4"
@ -1597,6 +1618,12 @@ version = "0.8.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "7a66a03ae7c801facd77a29370b4faec201768915ac14a721ba36f20bc9c209b"

+[[package]]
+name = "relative-path"
+version = "1.9.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "ba39f3699c378cd8970968dcbff9c43159ea4cfbd88d43c00b22f2ef10a435d2"
+
 [[package]]
 name = "reqwest"
 version = "0.11.27"
@ -1671,6 +1698,36 @@ dependencies = [
 "serde",
 ]

+[[package]]
+name = "rstest"
+version = "0.21.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9afd55a67069d6e434a95161415f5beeada95a01c7b815508a82dcb0e1593682"
+dependencies = [
+ "futures",
+ "futures-timer",
+ "rstest_macros",
+ "rustc_version",
+]
+
+[[package]]
+name = "rstest_macros"
+version = "0.21.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "4165dfae59a39dd41d8dec720d3cbfbc71f69744efb480a3920f5d4e0cc6798d"
+dependencies = [
+ "cfg-if",
+ "glob",
+ "proc-macro-crate",
+ "proc-macro2",
+ "quote",
+ "regex",
+ "relative-path",
+ "rustc_version",
+ "syn 2.0.52",
+ "unicode-ident",
+]
+
 [[package]]
 name = "rustc-demangle"
 version = "0.1.24"
@ -2221,6 +2278,7 @@ dependencies = [
 "pretty_env_logger 0.5.0",
 "rand",
 "reqwest",
+ "rstest",
 "serde",
 "serde_cbor",
 "serde_json",
@ -2411,6 +2469,23 @@ dependencies = [
 "tokio",
 ]

+[[package]]
+name = "toml_datetime"
+version = "0.6.7"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f8fb9f64314842840f1d940ac544da178732128f1c78c21772e876579e0da1db"
+
+[[package]]
+name = "toml_edit"
+version = "0.21.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "6a8534fd7f78b5405e860340ad6575217ce99f38d4d5c8f2442cb5ecb50090e1"
+dependencies = [
+ "indexmap 2.2.5",
+ "toml_datetime",
+ "winnow",
+]
+
 [[package]]
 name = "tower"
 version = "0.4.13"
@ -2897,6 +2972,15 @@ version = "0.52.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "32b752e52a2da0ddfbdbcc6fceadfeede4c939ed16d13e648833a61dfb611ed8"

+[[package]]
+name = "winnow"
+version = "0.5.40"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f593a95398737aeed53e489c785df13f3618e41dbcd6718c6addbf1395aa6876"
+dependencies = [
+ "memchr",
+]
+
 [[package]]
 name = "winreg"
 version = "0.50.0"
--- a/crates/teloxide-core/CHANGELOG.md
+++ b/crates/teloxide-core/CHANGELOG.md
@ -107,6 +107,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Deserialization of `ApiError::CantParseEntities` ([#839][pr839])
 - Deserialization of empty (content-less) messages that can sometimes appear as a part of callback query ([#850][pr850], issue [#873][issue873])
 - Type of `chat_id` in `send_game`: `u32` => `ChatId` ([#1066][pr1066])
+- In case of `RetryAfter(..)` errors the polling is paused for the specified delay instead of falling into the backoff strategy ([#1112][pr1112])

 [pr839]: https://github.com/teloxide/teloxide/pull/839
 [pr879]: https://github.com/teloxide/teloxide/pull/879
@ -116,6 +117,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [pr990]: https://github.com/teloxide/teloxide/pull/990
 [pr990]: https://github.com/teloxide/teloxide/pull/990
 [pr1066]: https://github.com/teloxide/teloxide/pull/1066
+[pr1112]: https://github.com/teloxide/teloxide/pull/1112

 ### Changed

--- a/crates/teloxide-core/src/adaptors/erased.rs
+++ b/crates/teloxide-core/src/adaptors/erased.rs
@ -4,6 +4,7 @@ use futures::{future::BoxFuture, FutureExt};
 use reqwest::Url;

 use crate::{
+    errors::AsResponseParameters,
    payloads::*,
    requests::{HasPayload, Output, Payload, Request, Requester},
    types::*,
@ -176,7 +177,7 @@ macro_rules! fwd_erased {

 impl<'a, Err> Requester for ErasedRequester<'a, Err>
 where
-    Err: std::error::Error + Send,
+    Err: std::error::Error + Send + AsResponseParameters,
 {
    type Err = Err;

--- a/crates/teloxide-core/src/requests/requester.rs
+++ b/crates/teloxide-core/src/requests/requester.rs
@ -3,7 +3,7 @@

 use url::Url;

-use crate::{payloads::*, requests::Request, types::*};
+use crate::{errors::AsResponseParameters, payloads::*, requests::Request, types::*};

 /// Telegram Bot API client.
 ///
@ -142,7 +142,7 @@ use crate::{payloads::*, requests::Request, types::*};
 #[cfg_attr(all(any(docsrs, dep_docsrs), feature = "nightly"), doc(notable_trait))]
 pub trait Requester {
    /// Error type returned by all requests.
-    type Err: std::error::Error + Send;
+    type Err: std::error::Error + Send + AsResponseParameters;

    // START BLOCK requester_methods
    // Generated by `codegen_requester_methods`, do not edit by hand.
--- a/crates/teloxide/Cargo.toml
+++ b/crates/teloxide/Cargo.toml
@ -126,6 +126,7 @@ tokio = { version = "1.8", features = ["fs", "rt-multi-thread", "macros"] }
 reqwest = "0.11.11"
 chrono = "0.4"
 tokio-stream = "0.1"
+rstest = "0.21.0"


 [package.metadata.docs.rs]
--- a/crates/teloxide/src/backoff.rs
+++ b/crates/teloxide/src/backoff.rs
@ -2,14 +2,30 @@ use std::time::Duration;

 pub type BackoffStrategy = Box<dyn Send + Fn(u32) -> Duration>;

+const THRESHOLD_ERROR_QUANTITY: u32 = 6;
+
 /// Calculates the backoff time in seconds for exponential strategy with base 2
 ///
-/// The maximum duration is limited to a little less than half an hour (1024
-/// secs), so the successive timings are(in secs): 1, 2, 4, .., 1024, 1024, ..
+/// The maximum duration is limited to a little more than a minute: 64s, so the
+/// successive timings are: 1s, 2s, 4s, .., 64, .., 64
 ///
 /// More at: <https://en.wikipedia.org/wiki/Exponential_backoff#Exponential_backoff_algorithm>
 pub fn exponential_backoff_strategy(error_count: u32) -> Duration {
-    // The error_count has to be limited so as not to cause overflow: 2^10 = 1024 ~
-    // a little less than half an hour
-    Duration::from_secs(1_u64 << error_count.min(10))
+    // 2^6 = 64s ~ a little more than a minute
+    Duration::from_secs(1_u64 << error_count.min(THRESHOLD_ERROR_QUANTITY))
+}
+
+#[cfg(test)]
+mod tests {
+    use rstest::rstest;
+
+    use super::*;
+
+    #[rstest]
+    #[case(1, Duration::from_secs(2))]
+    #[case(5, Duration::from_secs(32))]
+    #[case(42, Duration::from_secs(64))]
+    fn test_exponential_backoff_strategy(#[case] error_count: u32, #[case] expected: Duration) {
+        assert_eq!(exponential_backoff_strategy(error_count), expected);
+    }
 }
--- a/crates/teloxide/src/repls/commands_repl.rs
+++ b/crates/teloxide/src/repls/commands_repl.rs
@ -1,3 +1,8 @@
+use std::fmt::Debug;
+
+use dptree::di::{DependencyMap, Injectable};
+use futures::future::BoxFuture;
+
 use crate::{
    dispatching::{HandlerExt, UpdateFilterExt},
    error_handlers::LoggingErrorHandler,
@ -6,9 +11,6 @@ use crate::{
    update_listeners::{self, UpdateListener},
    utils::command::BotCommands,
 };
-use dptree::di::{DependencyMap, Injectable};
-use futures::future::BoxFuture;
-use std::fmt::Debug;

 /// A [REPL] for commands.
 ///
--- a/crates/teloxide/src/update_listeners/polling.rs
+++ b/crates/teloxide/src/update_listeners/polling.rs
@ -13,6 +13,8 @@ use std::{
 use futures::{ready, stream::Stream};
 use tokio::time::{sleep, Sleep};

+use teloxide_core::errors::AsResponseParameters;
+
 use crate::{
    backoff::{exponential_backoff_strategy, BackoffStrategy},
    requests::{HasPayload, Request, Requester},
@ -436,10 +438,22 @@ impl<B: Requester> Stream for PollingStream<'_, B> {
                    }
                }
                Err(err) => {
-                    // Prevents the CPU spike occuring at network connection lose: <https://github.com/teloxide/teloxide/issues/780>
-                    let backoff_strategy = &this.polling.backoff_strategy;
-                    this.eepy.set(Some(sleep(backoff_strategy(*this.error_count))));
-                    log::trace!("set {:?} reconnection delay", backoff_strategy(*this.error_count));
+                    /*
+                       In case of RetryAfter(..) error we pause the polling for the specified amount
+                       of seconds.
+
+                       Otherwise, in case of network connection lose (<https://github.com/teloxide/teloxide/issues/780>) we
+                       use the backoff strategy to prevent the high CPU usage due to multiple instant reconnections
+                    */
+                    if let Some(seconds) = err.retry_after() {
+                        log::info!("got `RetryAfter({})` error, polling paused", seconds);
+                        this.eepy.set(Some(sleep(seconds.duration())));
+                    } else {
+                        let delay = (this.polling.backoff_strategy)(*this.error_count);
+                        log::info!("retrying getting updates in {}s", delay.as_secs());
+                        this.eepy.set(Some(sleep(delay)));
+                    }
+
                    return Ready(Some(Err(err)));
                }
            }