Extract detection of LOCK_FAILURE errors into an ExceptionHook

ExceptionHook allows implementors to detect and handle exceptions that
are caused by temporary errors, and hence should cause a retry of the
failed operation. We should make use of it to detect and handle
LOCK_FAILURE errors. This way the code to detect LOCK_FAILURE is in a
single place instead of in RetryHelper for change actions and in
RestApiServlet for REST requests. As it turns out the detection of
LOCK_FAILURE errors in these places was inconsistent, RetryHelper did
unwrapping for UpdateException and StorageException, while
RestApiServlet did unwrapping only for UpdateException. Due to this some
LOCK_FAILURE errors such as [1] didn't trigger request retries.

[1]
com.google.gerrit.exceptions.StorageException: Star change 133855 for account 6003 failed
        at com.google.gerrit.server.StarredChangesUtil.star(StarredChangesUtil.java:234)
        at com.google.gerrit.server.restapi.account.StarredChanges$Create.apply(StarredChanges.java:133)
        at com.google.gerrit.server.restapi.account.StarredChanges$Create.apply(StarredChanges.java:97)
        at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestCollectionCreateViewWithRetry$5(RestApiServlet.java:758)
        at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78)
        at com.github.rholder.retry.Retryer.call(Retryer.java:160)
        at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:417)
        at com.google.gerrit.server.update.RetryHelper.executeWithAttemptAndTimeoutCount(RetryHelper.java:368)
        at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:271)
        at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:820)
        at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestCollectionCreateViewWithRetry(RestApiServlet.java:753)
        at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:527)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
        ...
Caused by: com.google.gerrit.git.LockFailureException: Update star labels on ref refs/starred-changes/55/133855/6003 failed
        at com.google.gerrit.server.StarredChangesUtil.updateLabels(StarredChangesUtil.java:484)
        at com.google.gerrit.server.StarredChangesUtil.star(StarredChangesUtil.java:227)
        ... 208 more

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ia7162ee5441d72b9ed32ff5b145b93d8626713c6

java/com/google/gerrit/httpd/restapi/RestApiServlet.java

@@ -173,7 +173,6 @@ import java.util.Optional;
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.concurrent.TimeUnit;
-import java.util.function.Predicate;
 import java.util.regex.Pattern;
 import java.util.stream.Stream;
 import java.util.zip.GZIPOutputStream;
@@ -704,8 +703,7 @@ public class RestApiServlet extends HttpServlet {
         traceContext,
         globals.metrics.view(restCollection.getClass(), pluginName) + "#parse",
         ActionType.REST_READ_REQUEST,
-        () -> restCollection.parse(parentResource, id),
+        () -> restCollection.parse(parentResource, id));
-        noRetry());
   }
 
   private Response<?> invokeRestReadViewWithRetry(
@@ -720,8 +718,7 @@ public class RestApiServlet extends HttpServlet {
         traceContext,
         getViewName(viewData),
         ActionType.REST_READ_REQUEST,
-        () -> view.apply(rsrc),
+        () -> view.apply(rsrc));
-        noRetry());
   }
 
   private Response<?> invokeRestModifyViewWithRetry(
@@ -737,8 +734,7 @@ public class RestApiServlet extends HttpServlet {
         traceContext,
         getViewName(viewData),
         ActionType.REST_WRITE_REQUEST,
-        () -> view.apply(rsrc, inputRequestBody),
+        () -> view.apply(rsrc, inputRequestBody));
-        retryOnLockFailure());
   }
 
   private Response<?> invokeRestCollectionCreateViewWithRetry(
@@ -755,8 +751,7 @@ public class RestApiServlet extends HttpServlet {
         traceContext,
         getViewName(viewData),
         ActionType.REST_WRITE_REQUEST,
-        () -> view.apply(rsrc, path, inputRequestBody),
+        () -> view.apply(rsrc, path, inputRequestBody));
-        retryOnLockFailure());
   }
 
   private Response<?> invokeRestCollectionDeleteMissingViewWithRetry(
@@ -773,8 +768,7 @@ public class RestApiServlet extends HttpServlet {
         traceContext,
         getViewName(viewData),
         ActionType.REST_WRITE_REQUEST,
-        () -> view.apply(rsrc, path, inputRequestBody),
+        () -> view.apply(rsrc, path, inputRequestBody));
-        retryOnLockFailure());
   }
 
   private Response<?> invokeRestCollectionModifyViewWithRetry(
@@ -790,8 +784,7 @@ public class RestApiServlet extends HttpServlet {
         traceContext,
         getViewName(viewData),
         ActionType.REST_WRITE_REQUEST,
-        () -> view.apply(rsrc, inputRequestBody),
+        () -> view.apply(rsrc, inputRequestBody));
-        retryOnLockFailure());
   }
 
   private <T> T invokeRestEndpointWithRetry(
@@ -799,8 +792,7 @@ public class RestApiServlet extends HttpServlet {
       TraceContext traceContext,
       String caller,
       ActionType actionType,
-      Action<T> action,
+      Action<T> action)
-      Predicate<Throwable> retryExceptionPredicate)
       throws Exception {
     RetryHelper.Options.Builder retryOptionsBuilder = RetryHelper.options().caller(caller);
     if (!traceContext.isTracing()) {
@@ -817,8 +809,11 @@ public class RestApiServlet extends HttpServlet {
                   });
     }
     try {
+      // ExceptionHookImpl controls on which exceptions we retry.
+      // The passed in exceptionPredicate allows to define additional exceptions on which retry
+      // should happen, but here we have none (hence pass in "t -> false" as exceptionPredicate).
       return globals.retryHelper.execute(
-          actionType, action, retryOptionsBuilder.build(), retryExceptionPredicate);
+          actionType, action, retryOptionsBuilder.build(), t -> false);
     } finally {
       // If auto-tracing got triggered due to a non-recoverable failure, also trace the rest of
       // this request. This means logging is forced for all further log statements and the logs are
@@ -827,19 +822,6 @@ public class RestApiServlet extends HttpServlet {
     }
   }
 
-  private static Predicate<Throwable> noRetry() {
-    return t -> false;
-  }
-
-  private static Predicate<Throwable> retryOnLockFailure() {
-    return t -> {
-      if (t instanceof UpdateException) {
-        t = t.getCause();
-      }
-      return t instanceof LockFailureException;
-    };
-  }
-
   private String getViewName(ViewData viewData) {
     return viewData != null && viewData.view != null ? globals.metrics.view(viewData) : "_unknown";
   }

java/com/google/gerrit/server/ExceptionHookImpl.java

@@ -0,0 +1,47 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server;
+
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.git.LockFailureException;
+import com.google.gerrit.server.update.UpdateException;
+import java.util.Optional;
+import org.eclipse.jgit.lib.RefUpdate;
+
+/**
+ * Class to detect and handle exceptions that are caused by temporary errors, and hence should cause
+ * a retry of the failed operation.
+ */
+public class ExceptionHookImpl implements ExceptionHook {
+  @Override
+  public boolean shouldRetry(Throwable throwable) {
+    return isLockFailure(throwable);
+  }
+
+  @Override
+  public Optional<String> formatCause(Throwable throwable) {
+    if (isLockFailure(throwable)) {
+      return Optional.of(RefUpdate.Result.LOCK_FAILURE.name());
+    }
+    return Optional.empty();
+  }
+
+  private static boolean isLockFailure(Throwable throwable) {
+    if (throwable instanceof UpdateException || throwable instanceof StorageException) {
+      throwable = throwable.getCause();
+    }
+    return throwable instanceof LockFailureException;
+  }
+}

java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -79,6 +79,7 @@ import com.google.gerrit.server.CmdLineParserModule;
 import com.google.gerrit.server.CreateGroupPermissionSyncer;
 import com.google.gerrit.server.DynamicOptions;
 import com.google.gerrit.server.ExceptionHook;
+import com.google.gerrit.server.ExceptionHookImpl;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.RequestListener;
 import com.google.gerrit.server.TraceRequestListener;
@@ -393,6 +394,7 @@ public class GerritGlobalModule extends FactoryModule {
     DynamicSet.bind(binder(), RequestListener.class).to(TraceRequestListener.class);
     DynamicSet.setOf(binder(), ChangeETagComputation.class);
     DynamicSet.setOf(binder(), ExceptionHook.class);
+    DynamicSet.bind(binder(), ExceptionHook.class).to(ExceptionHookImpl.class);
     DynamicSet.setOf(binder(), MailSoyTemplateProvider.class);
 
     DynamicMap.mapOf(binder(), MailFilter.class);

java/com/google/gerrit/server/update/RetryHelper.java

@@ -34,7 +34,6 @@ import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.git.LockFailureException;
 import com.google.gerrit.metrics.Counter1;
 import com.google.gerrit.metrics.Counter2;
 import com.google.gerrit.metrics.Counter3;
@@ -57,7 +56,6 @@ import java.util.concurrent.ExecutionException;
 import java.util.function.Consumer;
 import java.util.function.Predicate;
 import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.RefUpdate;
 
 @Singleton
 public class RetryHelper {
@@ -284,15 +282,7 @@ public class RetryHelper {
       throws RestApiException, UpdateException {
     try {
       return execute(
-          ActionType.CHANGE_UPDATE,
+          ActionType.CHANGE_UPDATE, () -> changeAction.call(updateFactory), opts, t -> false);
-          () -> changeAction.call(updateFactory),
-          opts,
-          t -> {
-            if (t instanceof UpdateException || t instanceof StorageException) {
-              t = t.getCause();
-            }
-            return t instanceof LockFailureException;
-          });
     } catch (Throwable t) {
       Throwables.throwIfUnchecked(t);
       Throwables.throwIfInstanceOf(t, UpdateException.class);
@@ -390,9 +380,6 @@ public class RetryHelper {
       return formattedCause.get();
     }
 
-    if (t instanceof LockFailureException) {
-      return RefUpdate.Result.LOCK_FAILURE.name();
-    }
     return t.getClass().getSimpleName();
   }