From 638327161668cf7db9223eed380d8a6221af5458 Mon Sep 17 00:00:00 2001
From: Tyler Ouyang <tylerwowen@users.noreply.github.com>
Date: Thu, 14 Nov 2024 16:15:48 -0800
Subject: [PATCH] Fix RetryInterceptor (#1751)

Similar issue https://github.com/square/okhttp/issues/4986

```
ava.lang.IllegalStateException: cannot make a new request because the previous response is still open: please call response.close() at okhttp3.internal.connection.RealCall.enterNetworkInterceptorExchange(RealCall.kt:229) at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.kt:66) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at com.pinterest.teletraan.universal.http.HttpClient.lambda$new$0(HttpClient.java:81) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at com.pinterest.teletraan.universal.http.RetryInterceptor.intercept(RetryInterceptor.java:44) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at io.micrometer.core.instrument.binder.okhttp3.OkHttpObservationInterceptor.intercept(OkHttpObservationInterceptor.java:98) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at okhttp3.logging.HttpLoggingInterceptor.intercept(HttpLoggingInterceptor.kt:154) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at okhttp3.internal.connection.RealCall.getResponseWithInterceptorChain$okhttp(RealCall.kt:201) at okhttp3.internal.connection.RealCall.execute(RealCall.kt:154) at com.pinterest.teletraan.universal.http.HttpClient.makeCall(HttpClient.java:179) at com.pinterest.teletraan.universal.http.HttpClient.post(HttpClient.java:140) at com.pinterest.deployservice.rodimus.RodimusManagerImpl.getTerminatedHosts(RodimusManagerImpl.java:129) at
```

## Test plan
Updated test cases and confirmed that the exception is thrown if `response.close();` is not called.
---
 .../universal/http/RetryInterceptor.java      | 30 +++++--------------
 .../universal/http/HttpClientTest.java        |  2 +-
 2 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java
index 4d66e3f8d8..f702c1ea55 100644
--- a/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java
+++ b/deploy-service/universal/src/main/java/com/pinterest/teletraan/universal/http/RetryInterceptor.java
@@ -36,37 +36,23 @@ public RetryInterceptor(int maxRetries, long retryInterval) {
     @Override
     public Response intercept(Chain chain) throws IOException {
         Request request = chain.request();
-        Response response = null;
-        IOException lastException = null;
+        Response response = chain.proceed(request);
+        int tryCount = 1;
 
-        for (int i = 0; i < maxRetries; i++) {
+        while (shouldRetry(response) && tryCount < maxRetries) {
+            long backoff = (long) Math.pow(2, (tryCount - 1)) * retryInterval;
+            response.close();
             try {
-                response = chain.proceed(request);
-                if (response.isSuccessful()) {
-                    return response;
-                }
-            } catch (IOException e) {
-                lastException = e;
-            }
-
-            if (!shouldRetry(response) || i == maxRetries - 1) {
-                break;
-            }
-
-            try {
-                long backoff = (long) Math.pow(2, i) * retryInterval;
                 TimeUnit.MILLISECONDS.sleep(backoff);
             } catch (InterruptedException e) {
                 Thread.currentThread().interrupt();
                 throw new IOException("Retry interrupted", e);
             }
+            response = chain.proceed(request);
+            tryCount++;
         }
 
-        if (response != null) {
-            return response;
-        } else {
-            throw lastException != null ? lastException : new IOException("Unknown error");
-        }
+        return response;
     }
 
     private boolean shouldRetry(Response response) {
diff --git a/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java b/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java
index 599e3b3c6f..9204b14e32 100644
--- a/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java
+++ b/deploy-service/universal/src/test/java/com/pinterest/teletraan/universal/http/HttpClientTest.java
@@ -278,7 +278,7 @@ static class ServerErrorDispatcher extends Dispatcher {
 
         @Override
         public MockResponse dispatch(RecordedRequest request) {
-            return new MockResponse().setResponseCode(responseCode);
+            return new MockResponse().setResponseCode(responseCode).setBody(TEST_BODY);
         }
     }
 }