diff --git a/pom.xml b/pom.xml index 988b944..63a0a2c 100644 --- a/pom.xml +++ b/pom.xml @@ -110,9 +110,15 @@ ${zstdVersion} - com.squareup.okhttp + com.squareup.okhttp3 okhttp - 2.7.5 + 4.12.0 + + + com.squareup.okhttp3 + mockwebserver + 4.12.0 + test diff --git a/src/main/java/dev/zarr/zarrjava/store/HttpStore.java b/src/main/java/dev/zarr/zarrjava/store/HttpStore.java index c2cc568..2c1a1b2 100644 --- a/src/main/java/dev/zarr/zarrjava/store/HttpStore.java +++ b/src/main/java/dev/zarr/zarrjava/store/HttpStore.java @@ -1,6 +1,6 @@ package dev.zarr.zarrjava.store; -import com.squareup.okhttp.*; +import okhttp3.*; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -8,6 +8,7 @@ import java.io.IOException; import java.io.InputStream; import java.nio.ByteBuffer; +import java.time.Duration; public class HttpStore implements Store { @@ -17,8 +18,16 @@ public class HttpStore implements Store { private final String uri; public HttpStore(@Nonnull String uri) { - this.httpClient = new OkHttpClient(); + this(uri, 60, 3, 1000); + } + + public HttpStore(@Nonnull String uri, int timeoutSeconds, int maxRetries, long retryDelayMs) { this.uri = uri; + this.httpClient = new OkHttpClient.Builder() + .connectTimeout(Duration.ofSeconds(timeoutSeconds)) + .readTimeout(Duration.ofSeconds(timeoutSeconds)) + .addInterceptor(new RetryInterceptor(maxRetries, retryDelayMs)) + .build(); } String resolveKeys(String[] keys) { @@ -37,9 +46,7 @@ String resolveKeys(String[] keys) { @Nullable ByteBuffer get(Request request, String[] keys) { - Call call = httpClient.newCall(request); - try { - Response response = call.execute(); + try (Response response = httpClient.newCall(request).execute()) { if (!response.isSuccessful()) { if (response.code() == 404) { return null; @@ -49,12 +56,8 @@ ByteBuffer get(Request request, String[] keys) { keys, new IOException("HTTP request failed with status code: " + response.code() + " " + response.message())); } - try (ResponseBody body = response.body()) { - if (body == null) { - return null; - } - return ByteBuffer.wrap(body.bytes()); - } + ResponseBody body = response.body(); + return (body == null) ? null : ByteBuffer.wrap(body.bytes()); } catch (IOException e) { throw StoreException.readFailed(this.toString(), keys, e); } @@ -63,9 +66,7 @@ ByteBuffer get(Request request, String[] keys) { @Override public boolean exists(String[] keys) { Request request = new Request.Builder().head().url(resolveKeys(keys)).build(); - Call call = httpClient.newCall(request); - try { - Response response = call.execute(); + try (Response response = httpClient.newCall(request).execute()) { return response.isSuccessful(); } catch (IOException e) { return false; @@ -129,28 +130,33 @@ public InputStream getInputStream(String[] keys, long start, long end) { } Request request = new Request.Builder().url(resolveKeys(keys)).header( "Range", String.format("bytes=%d-%d", start, end - 1)).build(); - Call call = httpClient.newCall(request); + try { - Response response = call.execute(); + // We do NOT use try-with-resources here because the stream must remain open + Response response = httpClient.newCall(request).execute(); if (!response.isSuccessful()) { if (response.code() == 404) { + response.close(); return null; } - throw StoreException.readFailed( - this.toString(), - keys, - new IOException("HTTP request failed with status code: " + response.code() + " " + response.message())); + int code = response.code(); + String msg = response.message(); + response.close(); + throw StoreException.readFailed(this.toString(), keys, + new IOException("HTTP request failed with status code: " + code + " " + msg)); } + ResponseBody body = response.body(); - if (body == null) return null; - InputStream stream = body.byteStream(); + if (body == null) { + response.close(); + return null; + } - // Ensure closing the stream also closes the response - return new FilterInputStream(stream) { + return new FilterInputStream(body.byteStream()) { @Override public void close() throws IOException { super.close(); - body.close(); + response.close(); // Closes both body and underlying connection } }; } catch (IOException e) { @@ -169,9 +175,7 @@ public long getSize(String[] keys) { .header("Accept-Encoding", "identity") .build(); - Call call = httpClient.newCall(request); - try { - Response response = call.execute(); + try (Response response = httpClient.newCall(request).execute()) { if (!response.isSuccessful()) { return -1; } @@ -193,4 +197,44 @@ public long getSize(String[] keys) { new IOException("Failed to get content length from HTTP HEAD request to: " + url, e)); } } -} + + /** + * Internal interceptor to handle retries for all HttpStore requests. + */ + private static class RetryInterceptor implements Interceptor { + private final int maxRetries; + private final long delay; + + RetryInterceptor(int maxRetries, long delay) { + this.maxRetries = maxRetries; + this.delay = delay; + } + + @Override + @Nonnull + public Response intercept(@Nonnull Chain chain) throws IOException { + Request request = chain.request(); + IOException lastException = null; + + for (int i = 0; i <= maxRetries; i++) { + try { + if (i > 0) Thread.sleep(delay); + Response response = chain.proceed(request); + + // Retry on common transient server errors (502, 503, 504) + if (response.isSuccessful() || response.code() == 404 || i == maxRetries || response.code() < 500) { + return response; + } + response.close(); + } catch (IOException e) { + lastException = e; + if (i == maxRetries) throw e; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Retry interrupted", e); + } + } + throw lastException != null ? lastException : new IOException("Request failed after retries"); + } + } +} \ No newline at end of file diff --git a/src/test/java/dev/zarr/zarrjava/ZarrV3Test.java b/src/test/java/dev/zarr/zarrjava/ZarrV3Test.java index 601b6da..c1e87a9 100644 --- a/src/test/java/dev/zarr/zarrjava/ZarrV3Test.java +++ b/src/test/java/dev/zarr/zarrjava/ZarrV3Test.java @@ -411,7 +411,7 @@ public void testReadme2() throws IOException, ZarrException { } @ParameterizedTest - @ValueSource(strings = {"1", "2-2-1", "4-4-1", "16-16-4"}) + @ValueSource(strings = {"1", "16-16-4"}) public void testReadL4Sample(String mag) throws IOException, ZarrException { StoreHandle httpStoreHandle = new HttpStore("https://static.webknossos.org/data/zarr_v3/").resolve("l4_sample", "color", mag); StoreHandle localStoreHandle = new FilesystemStore(TESTDATA).resolve("l4_sample", "color", mag); diff --git a/src/test/java/dev/zarr/zarrjava/store/HttpStoreTest.java b/src/test/java/dev/zarr/zarrjava/store/HttpStoreTest.java index 0b33331..911d4bf 100644 --- a/src/test/java/dev/zarr/zarrjava/store/HttpStoreTest.java +++ b/src/test/java/dev/zarr/zarrjava/store/HttpStoreTest.java @@ -1,15 +1,23 @@ package dev.zarr.zarrjava.store; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.SocketPolicy; import dev.zarr.zarrjava.ZarrException; import dev.zarr.zarrjava.core.Array; -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Disabled; -import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.*; import java.io.IOException; +import java.util.logging.Level; +import java.util.logging.Logger; class HttpStoreTest extends StoreTest { + @BeforeEach + void setupLogging() { + Logger.getLogger(MockWebServer.class.getName()).setLevel(Level.SEVERE); + } + @Override StoreHandle storeHandleWithData() { return br00109990StoreHandle().resolve("c", "0", "0", "0"); @@ -26,7 +34,7 @@ Store storeWithArrays() { } StoreHandle br00109990StoreHandle() { - HttpStore httpStore = new dev.zarr.zarrjava.store.HttpStore("https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.5/idr0033A"); + HttpStore httpStore = new HttpStore("https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.5/idr0033A"); return httpStore.resolve("BR00109990_C2.zarr", "0", "0"); } @@ -36,6 +44,49 @@ public void testOpen() throws IOException, ZarrException { Assertions.assertArrayEquals(new long[]{5, 1552, 2080}, array.metadata().shape); } + @Test + public void testCustomParameters() { + HttpStore httpStore = new HttpStore("https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.5/idr0033A"); + Assertions.assertTrue(httpStore.resolve("BR00109990_C2.zarr", "0", "0", "c", "0", "0", "0").exists()); + Assertions.assertFalse(httpStore.resolve("nonexistent").exists()); + } + + @Test + public void testRetryOnTimeout() throws IOException { + try (MockWebServer server = new MockWebServer()) { + server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE)); + server.enqueue(new MockResponse().setBody("data").setResponseCode(200)); + server.start(); + HttpStore httpStore = new HttpStore(server.url("/").toString(), 1, 3, 10); + Assertions.assertNotNull(httpStore.get(new String[]{"path"})); + Assertions.assertEquals(2, server.getRequestCount()); + } + } + + @Test + public void testRetryExhausted() throws IOException { + try (MockWebServer server = new MockWebServer()) { + for (int i = 0; i < 3; i++) { + server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE)); + } + server.start(); + HttpStore httpStore = new HttpStore(server.url("/").toString(), 1, 2, 10); + Assertions.assertThrows(StoreException.class, () -> httpStore.get(new String[]{"path"})); + Assertions.assertEquals(3, server.getRequestCount()); + } + } + + @Test + public void testNoRetryOn404() throws IOException { + try (MockWebServer server = new MockWebServer()) { + server.enqueue(new MockResponse().setResponseCode(404)); + server.start(); + HttpStore httpStore = new HttpStore(server.url("/").toString(), 1, 3, 10); + Assertions.assertNull(httpStore.get(new String[]{"path"})); + Assertions.assertEquals(1, server.getRequestCount()); + } + } + @Override @Test @Disabled("List is not supported in HttpStore") @@ -54,5 +105,4 @@ public void testListedItemsExist() { public void testListChildren() { } - }