Skip to content

Commit 1883172

Browse files
brokkoli71brokkoli71
authored
Http Request Retries (#62)
* add retry and update okhttp version * reduce redundant slow tests
1 parent 4612451 commit 1883172

File tree

4 files changed

+137
-37
lines changed

4 files changed

+137
-37
lines changed

‎pom.xml‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,9 +110,15 @@
110110
<version>${zstdVersion}</version>
111111
</dependency>
112112
<dependency>
113-
<groupId>com.squareup.okhttp</groupId>
113+
<groupId>com.squareup.okhttp3</groupId>
114114
<artifactId>okhttp</artifactId>
115-
<version>2.7.5</version>
115+
<version>4.12.0</version>
116+
</dependency>
117+
<dependency>
118+
<groupId>com.squareup.okhttp3</groupId>
119+
<artifactId>mockwebserver</artifactId>
120+
<version>4.12.0</version>
121+
<scope>test</scope>
116122
</dependency>
117123
<!-- JUnit 4 dependency for backward compatibility if needed -->
118124
<dependency>

‎src/main/java/dev/zarr/zarrjava/store/HttpStore.java‎

Lines changed: 73 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
11
package dev.zarr.zarrjava.store;
22

3-
import com.squareup.okhttp.*;
3+
import okhttp3.*;
44

55
import javax.annotation.Nonnull;
66
import javax.annotation.Nullable;
77
import java.io.FilterInputStream;
88
import java.io.IOException;
99
import java.io.InputStream;
1010
import java.nio.ByteBuffer;
11+
import java.time.Duration;
1112

1213
public class HttpStore implements Store {
1314

@@ -17,8 +18,16 @@ public class HttpStore implements Store {
1718
private final String uri;
1819

1920
public HttpStore(@Nonnull String uri) {
20-
this.httpClient = new OkHttpClient();
21+
this(uri, 60, 3, 1000);
22+
}
23+
24+
public HttpStore(@Nonnull String uri, int timeoutSeconds, int maxRetries, long retryDelayMs) {
2125
this.uri = uri;
26+
this.httpClient = new OkHttpClient.Builder()
27+
.connectTimeout(Duration.ofSeconds(timeoutSeconds))
28+
.readTimeout(Duration.ofSeconds(timeoutSeconds))
29+
.addInterceptor(new RetryInterceptor(maxRetries, retryDelayMs))
30+
.build();
2231
}
2332

2433
String resolveKeys(String[] keys) {
@@ -37,9 +46,7 @@ String resolveKeys(String[] keys) {
3746

3847
@Nullable
3948
ByteBuffer get(Request request, String[] keys) {
40-
Call call = httpClient.newCall(request);
41-
try {
42-
Response response = call.execute();
49+
try (Response response = httpClient.newCall(request).execute()) {
4350
if (!response.isSuccessful()) {
4451
if (response.code() == 404) {
4552
return null;
@@ -49,12 +56,8 @@ ByteBuffer get(Request request, String[] keys) {
4956
keys,
5057
new IOException("HTTP request failed with status code: " + response.code() + " " + response.message()));
5158
}
52-
try (ResponseBody body = response.body()) {
53-
if (body == null) {
54-
return null;
55-
}
56-
return ByteBuffer.wrap(body.bytes());
57-
}
59+
ResponseBody body = response.body();
60+
return (body == null) ? null : ByteBuffer.wrap(body.bytes());
5861
} catch (IOException e) {
5962
throw StoreException.readFailed(this.toString(), keys, e);
6063
}
@@ -63,9 +66,7 @@ ByteBuffer get(Request request, String[] keys) {
6366
@Override
6467
public boolean exists(String[] keys) {
6568
Request request = new Request.Builder().head().url(resolveKeys(keys)).build();
66-
Call call = httpClient.newCall(request);
67-
try {
68-
Response response = call.execute();
69+
try (Response response = httpClient.newCall(request).execute()) {
6970
return response.isSuccessful();
7071
} catch (IOException e) {
7172
return false;
@@ -129,28 +130,33 @@ public InputStream getInputStream(String[] keys, long start, long end) {
129130
}
130131
Request request = new Request.Builder().url(resolveKeys(keys)).header(
131132
"Range", String.format("bytes=%d-%d", start, end - 1)).build();
132-
Call call = httpClient.newCall(request);
133+
133134
try {
134-
Response response = call.execute();
135+
// We do NOT use try-with-resources here because the stream must remain open
136+
Response response = httpClient.newCall(request).execute();
135137
if (!response.isSuccessful()) {
136138
if (response.code() == 404) {
139+
response.close();
137140
return null;
138141
}
139-
throw StoreException.readFailed(
140-
this.toString(),
141-
keys,
142-
new IOException("HTTP request failed with status code: " + response.code() + " " + response.message()));
142+
int code = response.code();
143+
String msg = response.message();
144+
response.close();
145+
throw StoreException.readFailed(this.toString(), keys,
146+
new IOException("HTTP request failed with status code: " + code + " " + msg));
143147
}
148+
144149
ResponseBody body = response.body();
145-
if (body == null) return null;
146-
InputStream stream = body.byteStream();
150+
if (body == null) {
151+
response.close();
152+
return null;
153+
}
147154

148-
// Ensure closing the stream also closes the response
149-
return new FilterInputStream(stream) {
155+
return new FilterInputStream(body.byteStream()) {
150156
@Override
151157
public void close() throws IOException {
152158
super.close();
153-
body.close();
159+
response.close(); // Closes both body and underlying connection
154160
}
155161
};
156162
} catch (IOException e) {
@@ -169,9 +175,7 @@ public long getSize(String[] keys) {
169175
.header("Accept-Encoding", "identity")
170176
.build();
171177

172-
Call call = httpClient.newCall(request);
173-
try {
174-
Response response = call.execute();
178+
try (Response response = httpClient.newCall(request).execute()) {
175179
if (!response.isSuccessful()) {
176180
return -1;
177181
}
@@ -193,4 +197,44 @@ public long getSize(String[] keys) {
193197
new IOException("Failed to get content length from HTTP HEAD request to: " + url, e));
194198
}
195199
}
196-
}
200+
201+
/**
202+
* Internal interceptor to handle retries for all HttpStore requests.
203+
*/
204+
private static class RetryInterceptor implements Interceptor {
205+
private final int maxRetries;
206+
private final long delay;
207+
208+
RetryInterceptor(int maxRetries, long delay) {
209+
this.maxRetries = maxRetries;
210+
this.delay = delay;
211+
}
212+
213+
@Override
214+
@Nonnull
215+
public Response intercept(@Nonnull Chain chain) throws IOException {
216+
Request request = chain.request();
217+
IOException lastException = null;
218+
219+
for (int i = 0; i <= maxRetries; i++) {
220+
try {
221+
if (i > 0) Thread.sleep(delay);
222+
Response response = chain.proceed(request);
223+
224+
// Retry on common transient server errors (502, 503, 504)
225+
if (response.isSuccessful() || response.code() == 404 || i == maxRetries || response.code() < 500) {
226+
return response;
227+
}
228+
response.close();
229+
} catch (IOException e) {
230+
lastException = e;
231+
if (i == maxRetries) throw e;
232+
} catch (InterruptedException e) {
233+
Thread.currentThread().interrupt();
234+
throw new IOException("Retry interrupted", e);
235+
}
236+
}
237+
throw lastException != null ? lastException : new IOException("Request failed after retries");
238+
}
239+
}
240+
}

‎src/test/java/dev/zarr/zarrjava/ZarrV3Test.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -411,7 +411,7 @@ public void testReadme2() throws IOException, ZarrException {
411411
}
412412

413413
@ParameterizedTest
414-
@ValueSource(strings = {"1", "2-2-1", "4-4-1", "16-16-4"})
414+
@ValueSource(strings = {"1", "16-16-4"})
415415
public void testReadL4Sample(String mag) throws IOException, ZarrException {
416416
StoreHandle httpStoreHandle = new HttpStore("https://static.webknossos.org/data/zarr_v3/").resolve("l4_sample", "color", mag);
417417
StoreHandle localStoreHandle = new FilesystemStore(TESTDATA).resolve("l4_sample", "color", mag);

‎src/test/java/dev/zarr/zarrjava/store/HttpStoreTest.java‎

Lines changed: 55 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,23 @@
11
package dev.zarr.zarrjava.store;
22

3+
import okhttp3.mockwebserver.MockResponse;
4+
import okhttp3.mockwebserver.MockWebServer;
5+
import okhttp3.mockwebserver.SocketPolicy;
36
import dev.zarr.zarrjava.ZarrException;
47
import dev.zarr.zarrjava.core.Array;
5-
import org.junit.jupiter.api.Assertions;
6-
import org.junit.jupiter.api.Disabled;
7-
import org.junit.jupiter.api.Test;
8+
import org.junit.jupiter.api.*;
89

910
import java.io.IOException;
11+
import java.util.logging.Level;
12+
import java.util.logging.Logger;
1013

1114
class HttpStoreTest extends StoreTest {
1215

16+
@BeforeEach
17+
void setupLogging() {
18+
Logger.getLogger(MockWebServer.class.getName()).setLevel(Level.SEVERE);
19+
}
20+
1321
@Override
1422
StoreHandle storeHandleWithData() {
1523
return br00109990StoreHandle().resolve("c", "0", "0", "0");
@@ -26,7 +34,7 @@ Store storeWithArrays() {
2634
}
2735

2836
StoreHandle br00109990StoreHandle() {
29-
HttpStore httpStore = new dev.zarr.zarrjava.store.HttpStore("https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.5/idr0033A");
37+
HttpStore httpStore = new HttpStore("https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.5/idr0033A");
3038
return httpStore.resolve("BR00109990_C2.zarr", "0", "0");
3139
}
3240

@@ -36,6 +44,49 @@ public void testOpen() throws IOException, ZarrException {
3644
Assertions.assertArrayEquals(new long[]{5, 1552, 2080}, array.metadata().shape);
3745
}
3846

47+
@Test
48+
public void testCustomParameters() {
49+
HttpStore httpStore = new HttpStore("https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.5/idr0033A");
50+
Assertions.assertTrue(httpStore.resolve("BR00109990_C2.zarr", "0", "0", "c", "0", "0", "0").exists());
51+
Assertions.assertFalse(httpStore.resolve("nonexistent").exists());
52+
}
53+
54+
@Test
55+
public void testRetryOnTimeout() throws IOException {
56+
try (MockWebServer server = new MockWebServer()) {
57+
server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE));
58+
server.enqueue(new MockResponse().setBody("data").setResponseCode(200));
59+
server.start();
60+
HttpStore httpStore = new HttpStore(server.url("/").toString(), 1, 3, 10);
61+
Assertions.assertNotNull(httpStore.get(new String[]{"path"}));
62+
Assertions.assertEquals(2, server.getRequestCount());
63+
}
64+
}
65+
66+
@Test
67+
public void testRetryExhausted() throws IOException {
68+
try (MockWebServer server = new MockWebServer()) {
69+
for (int i = 0; i < 3; i++) {
70+
server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE));
71+
}
72+
server.start();
73+
HttpStore httpStore = new HttpStore(server.url("/").toString(), 1, 2, 10);
74+
Assertions.assertThrows(StoreException.class, () -> httpStore.get(new String[]{"path"}));
75+
Assertions.assertEquals(3, server.getRequestCount());
76+
}
77+
}
78+
79+
@Test
80+
public void testNoRetryOn404() throws IOException {
81+
try (MockWebServer server = new MockWebServer()) {
82+
server.enqueue(new MockResponse().setResponseCode(404));
83+
server.start();
84+
HttpStore httpStore = new HttpStore(server.url("/").toString(), 1, 3, 10);
85+
Assertions.assertNull(httpStore.get(new String[]{"path"}));
86+
Assertions.assertEquals(1, server.getRequestCount());
87+
}
88+
}
89+
3990
@Override
4091
@Test
4192
@Disabled("List is not supported in HttpStore")
@@ -54,5 +105,4 @@ public void testListedItemsExist() {
54105
public void testListChildren() {
55106
}
56107

57-
58108
}

0 commit comments

Comments
 (0)