Skip to content

Commit c2224b1

Browse files
allanrogerrclaude
andcommitted
fix(retry): drop Retry.java; revert createBody bracket per r3206778779
Three changes: (1) Retry.java is folded into Http.RetryInterceptor as package-private static members (MAX_RETRY, DEFAULT_RETRY_UNIT_MS, DEFAULT_RETRY_CAP_MS, MAX_JITTER, RETRYABLE_HTTP_STATUS_CODES, isHttpStatusRetryable, isRequestErrorRetryable, exponentialBackoffMs). The class was a one-use container for the interceptor's helpers; folding shrinks the package surface. RetryTest call sites updated to Http.RetryInterceptor.X. (2) BaseS3Client.createBody save/restore bracket dropped (r3206778779, "It is not needed for Java"). For the file-PUT paths that go through MinioAsyncClient, the upstream brackets at L1995/L3540 leave the file pointer at start-of-payload, and the pre-computed Checksum.makeHeaders headers short-circuit every Checksum.update guard inside createBody -- so the pointer never moves there. Http.RequestBody.writeTo seeks to the captured start-of-payload on every attempt, so the retry interceptor's repeated writeTo calls replay the same bytes. createBody is now byte-identical to master. (3) Checksum.java reverted to master form. The positional FileChannel.read(ByteBuffer, long) variant added in 93e4f47 exists only to make the bracket in createBody redundant; once the bracket is gone, the variant is no longer needed and Checksum.update returns to the master file.read(buf, off, len) form. MinioAsyncClient L1995/L3540 brackets remain (they match master verbatim). Empirical validation -- two new tests in RetryTest: - testRetryWithFileBody_replaysSameBytesOnEveryAttempt: 8 KiB random payload, MockWebServer queue 503/503/503/200; asserts 4 PUT requests recorded and each body is byte-identical to the original payload. - testRetryWithFileBody_transportFailureReplaysFromStart: 4 KiB payload, DISCONNECT_AT_START followed by 200; asserts the retry attempt's body matches the original payload after seek-back. :api:check (compile + spotless + spotbugs + 30 RetryTest cases) PASS :api:javadoc PASS Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 93e4f47 commit c2224b1

6 files changed

Lines changed: 273 additions & 172 deletions

File tree

api/src/main/java/io/minio/BaseS3Client.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ public abstract class BaseS3Client implements AutoCloseable {
118118
* Maximum attempts per S3 request. Effective only on the SDK-default {@link OkHttpClient};
119119
* caller-supplied clients are used verbatim and the SDK does not modify their retry policy.
120120
*/
121-
volatile int maxRetries = Retry.MAX_RETRY;
121+
volatile int maxRetries = Http.RetryInterceptor.MAX_RETRY;
122122

123123
protected BaseS3Client(
124124
Http.BaseUrl baseUrl, Provider provider, OkHttpClient httpClient, boolean closeHttpClient) {

api/src/main/java/io/minio/Checksum.java

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -104,31 +104,12 @@ private static void update(
104104
size = buffer.length();
105105
}
106106

107-
// Positional reads keep this method side-effect-free on the file pointer; callers don't need
108-
// to bracket with seek/restore.
109-
long filePos = 0L;
110-
java.nio.channels.FileChannel channel = null;
111-
if (file != null) {
112-
try {
113-
filePos = file.getFilePointer();
114-
channel = file.getChannel();
115-
} catch (IOException e) {
116-
throw new MinioException(e);
117-
}
118-
}
119-
120107
byte[] buf16k = new byte[16384];
121108
long bytesRead = 0;
122109
while (bytesRead != size) {
123110
try {
124111
int length = (int) Math.min(size - bytesRead, buf16k.length);
125-
int n;
126-
if (file != null) {
127-
java.nio.ByteBuffer dst = java.nio.ByteBuffer.wrap(buf16k, 0, length);
128-
n = channel.read(dst, filePos + bytesRead);
129-
} else {
130-
n = stream.read(buf16k, 0, length);
131-
}
112+
int n = file != null ? file.read(buf16k, 0, length) : stream.read(buf16k, 0, length);
132113
if (n < 0) throw new MinioException("unexpected EOF");
133114
if (n != 0) {
134115
bytesRead += n;

api/src/main/java/io/minio/Http.java

Lines changed: 89 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
import java.security.KeyStoreException;
3636
import java.security.NoSuchAlgorithmException;
3737
import java.security.SecureRandom;
38+
import java.security.cert.CertPathBuilderException;
39+
import java.security.cert.CertPathValidatorException;
3840
import java.security.cert.CertificateException;
3941
import java.security.cert.CertificateFactory;
4042
import java.security.cert.X509Certificate;
@@ -52,6 +54,7 @@
5254
import java.util.Map;
5355
import java.util.Objects;
5456
import java.util.Set;
57+
import java.util.concurrent.ThreadLocalRandom;
5558
import java.util.concurrent.TimeUnit;
5659
import java.util.function.IntSupplier;
5760
import java.util.regex.Matcher;
@@ -61,6 +64,9 @@
6164
import javax.net.ssl.HostnameVerifier;
6265
import javax.net.ssl.KeyManagerFactory;
6366
import javax.net.ssl.SSLContext;
67+
import javax.net.ssl.SSLException;
68+
import javax.net.ssl.SSLHandshakeException;
69+
import javax.net.ssl.SSLPeerUnverifiedException;
6470
import javax.net.ssl.SSLSession;
6571
import javax.net.ssl.SSLSocketFactory;
6672
import javax.net.ssl.TrustManager;
@@ -611,11 +617,12 @@ public static OkHttpClient enableExternalCertificatesFromEnv(OkHttpClient client
611617
}
612618

613619
/**
614-
* Creates the SDK's default HTTP client with a fixed retry budget of {@link Retry#MAX_RETRY}. For
615-
* a runtime-tunable budget, use {@link #newDefaultClient(IntSupplier)}.
620+
* Creates the SDK's default HTTP client with a fixed retry budget of {@link
621+
* RetryInterceptor#MAX_RETRY}. For a runtime-tunable budget, use {@link
622+
* #newDefaultClient(IntSupplier)}.
616623
*/
617624
public static OkHttpClient newDefaultClient() {
618-
return newDefaultClient(() -> Retry.MAX_RETRY);
625+
return newDefaultClient(() -> RetryInterceptor.MAX_RETRY);
619626
}
620627

621628
/**
@@ -736,11 +743,85 @@ public static OkHttpClient setTimeout(
736743
* deliberately does not, in exchange for a simpler interceptor model.)
737744
*/
738745
public static class RetryInterceptor implements Interceptor {
746+
/** Default maximum number of attempts per request. */
747+
static final int MAX_RETRY = 10;
748+
749+
/** Base unit per retry attempt, in milliseconds. */
750+
static final long DEFAULT_RETRY_UNIT_MS = 200L;
751+
752+
/** Per-attempt sleep cap, in milliseconds. */
753+
static final long DEFAULT_RETRY_CAP_MS = 1_000L;
754+
755+
/** Maximum jitter fraction in {@code [0.0, 1.0]}. {@code 1.0} = full jitter. */
756+
static final double MAX_JITTER = 1.0;
757+
758+
/** Retryable HTTP status codes. */
759+
static final Set<Integer> RETRYABLE_HTTP_STATUS_CODES =
760+
ImmutableSet.of(
761+
408, // Request Timeout
762+
429, // Too Many Requests
763+
499, // Client Closed Request (nginx)
764+
500, // Internal Server Error
765+
502, // Bad Gateway
766+
503, // Service Unavailable
767+
504, // Gateway Timeout
768+
520); // Cloudflare unknown error
769+
770+
static boolean isHttpStatusRetryable(int code) {
771+
return RETRYABLE_HTTP_STATUS_CODES.contains(code);
772+
}
773+
774+
/**
775+
* Returns true if {@code e} represents a transient transport failure that should be retried.
776+
* TLS handshake failure, unknown-CA / cert-path errors, and the "server gave HTTP response to
777+
* HTTPS client" protocol mismatch are NOT retryable; everything else (connection reset, EOF,
778+
* server closed idle connection, socket timeout, …) is.
779+
*/
780+
static boolean isRequestErrorRetryable(IOException e) {
781+
if (e instanceof SSLHandshakeException) return false;
782+
if (e instanceof SSLPeerUnverifiedException) return false;
783+
if (e instanceof SSLException) {
784+
Throwable cause = e.getCause();
785+
if (cause instanceof CertPathBuilderException
786+
|| cause instanceof CertPathValidatorException
787+
|| cause instanceof CertificateException) {
788+
return false;
789+
}
790+
}
791+
String msg = e.getMessage();
792+
if (msg != null && msg.contains("server gave HTTP response to HTTPS client")) return false;
793+
return true;
794+
}
795+
796+
/**
797+
* Computes the exponential-backoff-with-full-jitter delay for retry {@code attempt} (0-indexed:
798+
* {@code 0} = before the second attempt, {@code 1} = before the third, …):
799+
*
800+
* <pre>
801+
* sleep = min(DEFAULT_RETRY_CAP_MS, DEFAULT_RETRY_UNIT_MS * 2^attempt)
802+
* sleep -= (long)(random.nextDouble() * sleep * MAX_JITTER) // full jitter when MAX_JITTER == 1.0
803+
* </pre>
804+
*
805+
* <p>With {@code MAX_JITTER == 1.0}, returns a value in {@code [1, min(cap, base *
806+
* 2^attempt)]}. The lower bound is {@code 1} rather than {@code 0} because {@link
807+
* java.util.concurrent.ThreadLocalRandom#nextDouble()} is in {@code [0.0, 1.0)} and the {@code
808+
* (long)} cast truncates {@code rand * sleep} to at most {@code sleep - 1}. This matches the
809+
* behaviour of minio-go's {@code exponentialBackoffWait}, which uses the same formula and
810+
* therefore the same bounds.
811+
*/
812+
static long exponentialBackoffMs(int attempt) {
813+
int exp = Math.min(Math.max(attempt, 0), 30);
814+
long sleep = DEFAULT_RETRY_UNIT_MS * (1L << exp);
815+
if (sleep > DEFAULT_RETRY_CAP_MS) sleep = DEFAULT_RETRY_CAP_MS;
816+
sleep -= (long) (ThreadLocalRandom.current().nextDouble() * (double) sleep * MAX_JITTER);
817+
return Math.max(0L, sleep);
818+
}
819+
739820
private final IntSupplier maxAttemptsSupplier;
740821

741-
/** Uses a fixed budget of {@link Retry#MAX_RETRY} attempts. */
822+
/** Uses a fixed budget of {@link #MAX_RETRY} attempts. */
742823
public RetryInterceptor() {
743-
this(() -> Retry.MAX_RETRY);
824+
this(() -> MAX_RETRY);
744825
}
745826

746827
/**
@@ -769,7 +850,7 @@ public okhttp3.Response intercept(Chain chain) throws IOException {
769850
}
770851

771852
if (attempt > 0) {
772-
long delayMs = Retry.exponentialBackoffMs(attempt - 1);
853+
long delayMs = exponentialBackoffMs(attempt - 1);
773854
if (delayMs > 0L) {
774855
try {
775856
Thread.sleep(delayMs);
@@ -791,12 +872,12 @@ public okhttp3.Response intercept(Chain chain) throws IOException {
791872
// A cancelled call surfaces as IOException with a message that would
792873
// otherwise pass `isRequestErrorRetryable`. Bail out instead.
793874
if (chain.call().isCanceled()) throw e;
794-
if (!Retry.isRequestErrorRetryable(e)) throw e;
875+
if (!isRequestErrorRetryable(e)) throw e;
795876
lastException = e;
796877
continue;
797878
}
798879

799-
if (Retry.isHttpStatusRetryable(response.code())) {
880+
if (isHttpStatusRetryable(response.code())) {
800881
lastException = null;
801882
continue;
802883
}

api/src/main/java/io/minio/MinioAsyncClient.java

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ public static final class Builder {
156156
private Provider provider;
157157
private OkHttpClient httpClient;
158158
private boolean closeHttpClient;
159-
private int maxRetries = Retry.MAX_RETRY;
159+
private int maxRetries = Http.RetryInterceptor.MAX_RETRY;
160160

161161
public Builder baseUrl(Http.BaseUrl baseUrl) {
162162
if (baseUrl.region() == null) {
@@ -1993,7 +1993,9 @@ private CompletableFuture<ObjectWriteResponse> putObject(
19931993
Map<Checksum.Algorithm, Checksum.Hasher> hashers = Checksum.newHasherMap(algorithms);
19941994

19951995
if (file != null) {
1996+
long position = file.getFilePointer();
19961997
Checksum.update(hashers, file, length);
1998+
file.seek(position);
19971999
return putObject(
19982000
new PutObjectAPIArgs(
19992001
args,
@@ -2013,6 +2015,8 @@ private CompletableFuture<ObjectWriteResponse> putObject(
20132015
Checksum.makeHeaders(hashers, addContentSha256, addSha256Checksum)));
20142016
} catch (MinioException e) {
20152017
return Utils.failedFuture(e);
2018+
} catch (IOException e) {
2019+
return Utils.failedFuture(new MinioException(e));
20162020
}
20172021
}
20182022

@@ -3540,9 +3544,13 @@ private CompletableFuture<ObjectWriteResponse> appendObject(
35403544
}
35413545

35423546
try {
3547+
long position = file.getFilePointer();
35433548
Checksum.update(hashers, file, size);
3549+
file.seek(position);
35443550
} catch (MinioException e) {
35453551
return Utils.failedFuture(e);
3552+
} catch (IOException e) {
3553+
return Utils.failedFuture(new MinioException(e));
35463554
}
35473555

35483556
headers.putAll(Checksum.makeHeaders(hashers, addContentSha256, addSha256Checksum));

api/src/main/java/io/minio/Retry.java

Lines changed: 0 additions & 110 deletions
This file was deleted.

0 commit comments

Comments
 (0)