From 270b6aeea59460f35d78f8c5eacae954008a2869 Mon Sep 17 00:00:00 2001
From: Daniel Szoke <daniel.szoke@sentry.io>
Date: Tue, 27 May 2025 12:25:24 +0200
Subject: [PATCH] ref: Only obtain max retry count once

With the current implementation, we attempt to retrieve the max retry count from the environment or from the ini file on every access, which could lead to the user being warned multiple times if an invalid value is supplied. Instead, we should only obtain this count once and store it in the `Config`
---
 src/api/mod.rs | 12 ++++++------
 src/config.rs  | 53 ++++++++++++++++++++++++++++++--------------------
 2 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/src/api/mod.rs b/src/api/mod.rs
index 6cfc7454fb..d99ffa103c 100644
--- a/src/api/mod.rs
+++ b/src/api/mod.rs
@@ -396,7 +396,7 @@ impl Api {
             .request(Method::Post, url, None)?
             .with_form_data(form)?
             .with_retry(
-                self.config.get_max_retry_count(),
+                self.config.max_retries(),
                 &[
                     http::HTTP_STATUS_502_BAD_GATEWAY,
                     http::HTTP_STATUS_503_SERVICE_UNAVAILABLE,
@@ -968,7 +968,7 @@ impl<'a> AuthenticatedApi<'a> {
         self.request(Method::Post, &url)?
             .with_json_body(request)?
             .with_retry(
-                self.api.config.get_max_retry_count(),
+                self.api.config.max_retries(),
                 &[
                     http::HTTP_STATUS_502_BAD_GATEWAY,
                     http::HTTP_STATUS_503_SERVICE_UNAVAILABLE,
@@ -1001,7 +1001,7 @@ impl<'a> AuthenticatedApi<'a> {
                 dist: None,
             })?
             .with_retry(
-                self.api.config.get_max_retry_count(),
+                self.api.config.max_retries(),
                 &[
                     http::HTTP_STATUS_502_BAD_GATEWAY,
                     http::HTTP_STATUS_503_SERVICE_UNAVAILABLE,
@@ -1032,7 +1032,7 @@ impl<'a> AuthenticatedApi<'a> {
                 dist,
             })?
             .with_retry(
-                self.api.config.get_max_retry_count(),
+                self.api.config.max_retries(),
                 &[
                     http::HTTP_STATUS_502_BAD_GATEWAY,
                     http::HTTP_STATUS_503_SERVICE_UNAVAILABLE,
@@ -1408,7 +1408,7 @@ impl RegionSpecificApi<'_> {
         self.request(Method::Post, &path)?
             .with_form_data(form)?
             .with_retry(
-                self.api.api.config.get_max_retry_count(),
+                self.api.api.config.max_retries(),
                 &[http::HTTP_STATUS_507_INSUFFICIENT_STORAGE],
             )
             .progress_bar_mode(ProgressBarMode::Request)
@@ -1467,7 +1467,7 @@ impl RegionSpecificApi<'_> {
             .request(Method::Post, &path)?
             .with_form_data(form)?
             .with_retry(
-                self.api.api.config.get_max_retry_count(),
+                self.api.api.config.max_retries(),
                 &[
                     http::HTTP_STATUS_502_BAD_GATEWAY,
                     http::HTTP_STATUS_503_SERVICE_UNAVAILABLE,
diff --git a/src/config.rs b/src/config.rs
index db5125a853..289a2e8d92 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -53,6 +53,7 @@ pub struct Config {
     cached_log_level: log::LevelFilter,
     cached_vcs_remote: String,
     cached_token_data: Option<AuthTokenPayload>,
+    max_retries: u32,
 }
 
 impl Config {
@@ -91,6 +92,7 @@ impl Config {
             cached_headers: get_default_headers(&ini),
             cached_log_level: get_default_log_level(&ini),
             cached_vcs_remote: get_default_vcs_remote(&ini),
+            max_retries: get_max_retries(&ini),
             ini,
             cached_token_data: token_embedded_data,
         }
@@ -468,27 +470,8 @@ impl Config {
     }
 
     /// Returns the configured maximum number of retries for failed HTTP requests.
-    pub fn get_max_retry_count(&self) -> u32 {
-        match max_retries_from_env() {
-            Ok(Some(val)) => return val,
-            Ok(None) => (),
-            Err(e) => {
-                warn!(
-                    "Ignoring invalid {MAX_RETRIES_ENV_VAR} environment variable: {}",
-                    e
-                );
-            }
-        };
-
-        match max_retries_from_ini(&self.ini) {
-            Ok(Some(val)) => return val,
-            Ok(None) => (),
-            Err(e) => {
-                warn!("Ignoring invalid {MAX_RETRIES_INI_KEY} ini key: {}", e);
-            }
-        };
-
-        DEFAULT_RETRIES
+    pub fn max_retries(&self) -> u32 {
+        self.max_retries
     }
 
     /// Return the DSN
@@ -539,6 +522,32 @@ impl Config {
     }
 }
 
+/// Obtains the maximum number of retries from the environment or the ini file.
+/// Environment variable takes precedence over the ini file. If neither is set,
+/// the default value is returned.
+fn get_max_retries(ini: &Ini) -> u32 {
+    match max_retries_from_env() {
+        Ok(Some(val)) => return val,
+        Ok(None) => (),
+        Err(e) => {
+            warn!(
+                "Ignoring invalid {MAX_RETRIES_ENV_VAR} environment variable: {}",
+                e
+            );
+        }
+    };
+
+    match max_retries_from_ini(ini) {
+        Ok(Some(val)) => return val,
+        Ok(None) => (),
+        Err(e) => {
+            warn!("Ignoring invalid {MAX_RETRIES_INI_KEY} ini key: {}", e);
+        }
+    };
+
+    DEFAULT_RETRIES
+}
+
 /// Computes the maximum number of retries from the `SENTRY_HTTP_MAX_RETRIES` environment variable.
 /// Returns `Ok(None)` if the environment variable is not set, other errors are returned as is.
 fn max_retries_from_env() -> Result<Option<u32>> {
@@ -709,6 +718,7 @@ impl Clone for Config {
             cached_log_level: self.cached_log_level,
             cached_vcs_remote: self.cached_vcs_remote.clone(),
             cached_token_data: self.cached_token_data.clone(),
+            max_retries: self.max_retries,
         }
     }
 }
@@ -794,6 +804,7 @@ mod tests {
             cached_log_level: LevelFilter::Off,
             cached_vcs_remote: String::new(),
             cached_token_data: None,
+            max_retries: 0,
         };
 
         assert_eq!(