fix: Improve remote evaluation fetch retry logic (#49)

amplitude · Jan 29, 2024 · c172d15 · c172d15
1 parent 2bd41b2
commit c172d15
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 1 deletion.
diff --git a/sdk/src/main/java/com/amplitude/experiment/DefaultExperimentClient.kt b/sdk/src/main/java/com/amplitude/experiment/DefaultExperimentClient.kt
@@ -11,6 +11,7 @@ import com.amplitude.experiment.storage.getVariantStorage
 import com.amplitude.experiment.util.AsyncFuture
 import com.amplitude.experiment.util.Backoff
 import com.amplitude.experiment.util.BackoffConfig
+import com.amplitude.experiment.util.FetchException
 import com.amplitude.experiment.util.Logger
 import com.amplitude.experiment.util.Poller
 import com.amplitude.experiment.util.SessionAnalyticsProvider
@@ -36,6 +37,7 @@ import org.json.JSONObject
 import java.io.IOException
 import java.lang.IllegalStateException
 import java.util.concurrent.Callable
+import java.util.concurrent.ExecutionException
 import java.util.concurrent.Future
 import java.util.concurrent.ScheduledExecutorService
 import java.util.concurrent.TimeUnit
@@ -293,7 +295,7 @@ internal class DefaultExperimentClient internal constructor(
             val variants = doFetch(user, timeoutMillis, options).get()
             storeVariants(variants, options)
         } catch (e: Exception) {
-            if (retry) {
+            if (retry && shouldRetryFetch(e)) {
                 startRetries(user, options)
             }
             throw e
@@ -339,6 +341,9 @@ internal class DefaultExperimentClient internal constructor(
             override fun onResponse(call: Call, response: Response) {
                 try {
                     Logger.d("Received fetch variants response: $response")
+                    if (!response.isSuccessful) {
+                        throw FetchException(response.code, "fetch error response: $response")
+                    }
                     val variants = parseResponse(response)
                     future.complete(variants)
                 } catch (e: Exception) {
@@ -664,6 +669,14 @@ internal class DefaultExperimentClient internal constructor(
             }
         }
     }
+
+    private fun shouldRetryFetch(e: Exception): Boolean {
+        if (e is ExecutionException && e.cause is FetchException) {
+            val fetchException = e.cause as FetchException
+            return fetchException.statusCode < 400 || fetchException.statusCode >= 500 || fetchException.statusCode == 429
+        }
+        return true
+    }
 }
 
 data class VariantAndSource(

diff --git a/sdk/src/main/java/com/amplitude/experiment/util/FetchException.kt b/sdk/src/main/java/com/amplitude/experiment/util/FetchException.kt
@@ -0,0 +1,8 @@
+package com.amplitude.experiment.util
+
+import okio.IOException
+
+internal class FetchException(
+    val statusCode: Int,
+    message: String
+) : IOException(message)
diff --git a/sdk/src/test/java/com/amplitude/experiment/ExperimentClientTest.kt b/sdk/src/test/java/com/amplitude/experiment/ExperimentClientTest.kt
@@ -2,6 +2,7 @@ package com.amplitude.experiment
 
 import com.amplitude.experiment.analytics.ExperimentAnalyticsEvent
 import com.amplitude.experiment.analytics.ExperimentAnalyticsProvider
+import com.amplitude.experiment.util.FetchException
 import com.amplitude.experiment.util.Logger
 import com.amplitude.experiment.util.MockStorage
 import com.amplitude.experiment.util.SystemLogger
@@ -19,6 +20,7 @@ import org.json.JSONObject
 import org.junit.Assert
 import org.junit.Before
 import org.junit.Test
+import java.util.concurrent.CompletableFuture
 import java.util.concurrent.ExecutionException
 
 private const val API_KEY = "client-DvWljIjiiuqLbyjqdvBaLFfEBrAvGuA3"
@@ -1200,4 +1202,48 @@ class ExperimentClientTest {
         variant2 = client.variant("sdk-ci-test-local-2")
         Assert.assertEquals("on", variant2.key)
     }
+
+    @Test
+    fun `fetch retry with different response codes`() {
+        // Response code, error message, and whether retry should be called
+        val testData = listOf(
+            Triple(300, "Fetch Exception 300", 1),
+            Triple(400, "Fetch Exception 400", 0),
+            Triple(429, "Fetch Exception 429", 1),
+            Triple(500, "Fetch Exception 500", 1),
+            Triple(0, "Other Exception", 1)
+        )
+
+        testData.forEach { (responseCode, errorMessage, retryCalled) ->
+            val storage = MockStorage()
+            val client = spyk(
+                DefaultExperimentClient(
+                    API_KEY,
+                    ExperimentConfig(retryFetchOnFailure = true),
+                    OkHttpClient(),
+                    storage,
+                    Experiment.executorService,
+                ),
+                recordPrivateCalls = true
+            )
+            // Mock the private method to throw FetchException or other exceptions
+            every { client["doFetch"](any<ExperimentUser>(), any<Long>(), any<FetchOptions>()) } answers {
+                val future = CompletableFuture<Map<String, Variant>>()
+                if (responseCode == 0) {
+                    future.completeExceptionally(Exception(errorMessage))
+                } else {
+                    future.completeExceptionally(FetchException(responseCode, errorMessage))
+                }
+                future
+            }
+
+            try {
+                client.fetch(ExperimentUser("test_user")).get()
+            } catch (t: Throwable) {
+                // Ignore exception
+            }
+
+            verify(exactly = retryCalled) { client["startRetries"](any<ExperimentUser>(), any<FetchOptions>()) }
+        }
+    }
 }