Copilot commented on code in PR #6992:
URL: https://github.com/apache/ignite-3/pull/6992#discussion_r2533750200
##########
modules/client/src/main/java/org/apache/ignite/internal/client/ClientFutureUtils.java:
##########
@@ -37,54 +37,49 @@ class ClientFutureUtils {
static <T> CompletableFuture<T> doWithRetryAsync(
Supplier<CompletableFuture<T>> func,
- @Nullable Predicate<T> resultValidator,
Predicate<RetryContext> retryPredicate) {
CompletableFuture<T> resFut = new CompletableFuture<>();
RetryContext ctx = new RetryContext();
- doWithRetryAsync(func, resultValidator, retryPredicate, resFut, ctx);
+ doWithRetryAsync(func, retryPredicate, resFut, ctx);
return resFut;
}
private static <T> void doWithRetryAsync(
Supplier<CompletableFuture<T>> func,
- @Nullable Predicate<T> validator,
Predicate<RetryContext> retryPredicate,
CompletableFuture<T> resFut,
RetryContext ctx) {
func.get().whenComplete((res, err) -> {
try {
- if (err == null && (validator == null || validator.test(res)))
{
+ if (err == null) {
resFut.complete(res);
return;
}
- if (err != null) {
+ synchronized (ctx) {
if (ctx.errors == null) {
ctx.errors = new ArrayList<>();
}
ctx.errors.add(err);
- }
- if (retryPredicate.test(ctx)) {
- ctx.attempt++;
+ if (retryPredicate.test(ctx)) {
+ ctx.attempt++;
+
+ doWithRetryAsync(func, retryPredicate, resFut, ctx);
- doWithRetryAsync(func, validator, retryPredicate, resFut,
ctx);
- } else {
- if (ctx.errors == null || ctx.errors.isEmpty()) {
- // Should not happen.
- resFut.completeExceptionally(new
IllegalStateException("doWithRetry failed without exception"));
- } else {
- var resErr = ctx.errors.get(0);
+ return;
+ }
- for (int i = 1; i < ctx.errors.size(); i++) {
- resErr.addSuppressed(ctx.errors.get(i));
- }
+ Throwable resErr = ctx.errors.get(0);
- resFut.completeExceptionally(resErr);
+ for (int i = 1; i < ctx.errors.size(); i++) {
+ resErr.addSuppressed(ctx.errors.get(i));
}
+
+ resFut.completeExceptionally(resErr);
Review Comment:
The synchronized block on `ctx` is unnecessarily broad. The
`doWithRetryAsync` recursive call at line 71 happens inside the synchronized
block, which can lead to nested synchronization and potential performance
issues. Only the operations that modify `ctx.errors` and `ctx.attempt` need
synchronization. Consider releasing the lock before making the recursive call
by restructuring the code to synchronize only around the mutation operations
and determine retry decision, then execute the retry outside the lock.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
