Merge changes I0faffdd1,I795da1fb,Id926b849,I2f584fd0,Ic653584f
* changes:
Record correct status in REST metric if ExceptionHook changed the status
ExceptionHook: Pass trace ID into getUserMessages method
ExceptionHook: Allow to return multiple user messages
Control response code for LOCK_FAILUREs from ExceptionHookImpl
ExceptionHook: Document the behavior when multiple exception hooks are registered
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 51a6191..d9bb3b9 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -45,7 +45,6 @@
import static javax.servlet.http.HttpServletResponse.SC_NOT_MODIFIED;
import static javax.servlet.http.HttpServletResponse.SC_OK;
import static javax.servlet.http.HttpServletResponse.SC_PRECONDITION_FAILED;
-import static javax.servlet.http.HttpServletResponse.SC_SERVICE_UNAVAILABLE;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.CharMatcher;
@@ -105,7 +104,6 @@
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.ExceptionHook;
-import com.google.gerrit.server.ExceptionHookImpl;
import com.google.gerrit.server.OptionUtil;
import com.google.gerrit.server.RequestInfo;
import com.google.gerrit.server.RequestListener;
@@ -318,7 +316,7 @@
long auditStartTs = TimeUtil.nowMs();
res.setHeader("Content-Disposition", "attachment");
res.setHeader("X-Content-Type-Options", "nosniff");
- int status = SC_OK;
+ int statusCode = SC_OK;
long responseBytes = -1;
Optional<Exception> cause = Optional.empty();
Response<?> response = null;
@@ -587,10 +585,10 @@
return;
}
- status = response.statusCode();
+ statusCode = response.statusCode();
configureCaching(req, res, traceContext, rsrc, viewData, response.caching());
- res.setStatus(status);
- logger.atFinest().log("REST call succeeded: %d", status);
+ res.setStatus(statusCode);
+ logger.atFinest().log("REST call succeeded: %d", statusCode);
}
if (response != Response.none()) {
@@ -605,44 +603,48 @@
cause = Optional.of(e);
responseBytes =
replyError(
- req, res, status = SC_BAD_REQUEST, "Invalid " + JSON_TYPE + " in request", e);
+ req, res, statusCode = SC_BAD_REQUEST, "Invalid " + JSON_TYPE + " in request", e);
} catch (BadRequestException e) {
cause = Optional.of(e);
responseBytes =
replyError(
- req, res, status = SC_BAD_REQUEST, messageOr(e, "Bad Request"), e.caching(), e);
+ req, res, statusCode = SC_BAD_REQUEST, messageOr(e, "Bad Request"), e.caching(), e);
} catch (AuthException e) {
cause = Optional.of(e);
responseBytes =
- replyError(req, res, status = SC_FORBIDDEN, messageOr(e, "Forbidden"), e.caching(), e);
+ replyError(
+ req, res, statusCode = SC_FORBIDDEN, messageOr(e, "Forbidden"), e.caching(), e);
} catch (AmbiguousViewException e) {
cause = Optional.of(e);
- responseBytes = replyError(req, res, status = SC_NOT_FOUND, messageOr(e, "Ambiguous"), e);
+ responseBytes =
+ replyError(req, res, statusCode = SC_NOT_FOUND, messageOr(e, "Ambiguous"), e);
} catch (ResourceNotFoundException e) {
cause = Optional.of(e);
responseBytes =
- replyError(req, res, status = SC_NOT_FOUND, messageOr(e, "Not Found"), e.caching(), e);
+ replyError(
+ req, res, statusCode = SC_NOT_FOUND, messageOr(e, "Not Found"), e.caching(), e);
} catch (MethodNotAllowedException e) {
cause = Optional.of(e);
responseBytes =
replyError(
req,
res,
- status = SC_METHOD_NOT_ALLOWED,
+ statusCode = SC_METHOD_NOT_ALLOWED,
messageOr(e, "Method Not Allowed"),
e.caching(),
e);
} catch (ResourceConflictException e) {
cause = Optional.of(e);
responseBytes =
- replyError(req, res, status = SC_CONFLICT, messageOr(e, "Conflict"), e.caching(), e);
+ replyError(
+ req, res, statusCode = SC_CONFLICT, messageOr(e, "Conflict"), e.caching(), e);
} catch (PreconditionFailedException e) {
cause = Optional.of(e);
responseBytes =
replyError(
req,
res,
- status = SC_PRECONDITION_FAILED,
+ statusCode = SC_PRECONDITION_FAILED,
messageOr(e, "Precondition Failed"),
e.caching(),
e);
@@ -652,7 +654,7 @@
replyError(
req,
res,
- status = SC_UNPROCESSABLE_ENTITY,
+ statusCode = SC_UNPROCESSABLE_ENTITY,
messageOr(e, "Unprocessable Entity"),
e.caching(),
e);
@@ -660,34 +662,43 @@
cause = Optional.of(e);
logger.atSevere().withCause(e).log("Error in %s %s", req.getMethod(), uriForLogging(req));
responseBytes =
- replyError(req, res, status = SC_NOT_IMPLEMENTED, messageOr(e, "Not Implemented"), e);
+ replyError(
+ req, res, statusCode = SC_NOT_IMPLEMENTED, messageOr(e, "Not Implemented"), e);
} catch (QuotaException e) {
cause = Optional.of(e);
responseBytes =
replyError(
req,
res,
- status = SC_TOO_MANY_REQUESTS,
+ statusCode = SC_TOO_MANY_REQUESTS,
messageOr(e, "Quota limit reached"),
e.caching(),
e);
} catch (Exception e) {
cause = Optional.of(e);
+ statusCode = SC_INTERNAL_SERVER_ERROR;
- if (ExceptionHookImpl.isLockFailure(e)) {
- logger.atSevere().withCause(e.getCause()).log(
- "Error in %s %s", req.getMethod(), uriForLogging(req));
- responseBytes = replyError(req, res, status = SC_SERVICE_UNAVAILABLE, "Lock failure", e);
+ if (res.isCommitted()) {
+ logger.atSevere().withCause(e).log(
+ "Error in %s %s, response already committed", req.getMethod(), uriForLogging(req));
+ responseBytes = 0;
+ } else {
+ res.reset();
+ traceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
+
+ Optional<ExceptionHook.Status> status = getStatus(e);
+ if (status.isPresent()) {
+ statusCode = status.get().statusCode();
+ responseBytes = reply(req, res, e, status.get(), getUserMessages(traceContext, e));
+ }
+ responseBytes = replyInternalServerError(req, res, e, getUserMessages(traceContext, e));
}
-
- status = SC_INTERNAL_SERVER_ERROR;
- responseBytes = handleException(traceContext, e, req, res);
} finally {
String metric = getViewName(viewData);
String formattedCause = cause.map(globals.retryHelper::formatCause).orElse("_none");
globals.metrics.count.increment(metric);
- if (status >= SC_BAD_REQUEST) {
- globals.metrics.errorCount.increment(metric, status, formattedCause);
+ if (statusCode >= SC_BAD_REQUEST) {
+ globals.metrics.errorCount.increment(metric, statusCode, formattedCause);
}
if (responseBytes != -1) {
globals.metrics.responseBytes.record(metric, responseBytes);
@@ -702,7 +713,7 @@
auditStartTs,
qp != null ? qp.params() : ImmutableListMultimap.of(),
inputRequestBody,
- status,
+ statusCode,
response,
rsrc,
viewData == null ? null : viewData.view));
@@ -1736,50 +1747,60 @@
}
}
- private long handleException(
- TraceContext traceContext, Throwable err, HttpServletRequest req, HttpServletResponse res)
+ private Optional<ExceptionHook.Status> getStatus(Throwable err) {
+ return globals.exceptionHooks.stream()
+ .map(h -> h.getStatus(err))
+ .filter(Optional::isPresent)
+ .map(Optional::get)
+ .findFirst();
+ }
+
+ private ImmutableList<String> getUserMessages(TraceContext traceContext, Throwable err) {
+ return globals.exceptionHooks.stream()
+ .flatMap(h -> h.getUserMessages(err, traceContext.getTraceId().orElse(null)).stream())
+ .collect(toImmutableList());
+ }
+
+ private static long reply(
+ HttpServletRequest req,
+ HttpServletResponse res,
+ Throwable err,
+ ExceptionHook.Status status,
+ ImmutableList<String> userMessages)
throws IOException {
- if (res.isCommitted()) {
- logger.atSevere().withCause(err).log(
- "Error in %s %s, response already committed", req.getMethod(), uriForLogging(req));
- return 0;
+ res.setStatus(status.statusCode());
+
+ StringBuilder msg = new StringBuilder(status.statusMessage());
+ if (!userMessages.isEmpty()) {
+ msg.append("\n");
+ userMessages.forEach(m -> msg.append("\n* ").append(m));
}
- res.reset();
- traceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
- ImmutableList<String> userMessages =
- globals.exceptionHooks.stream()
- .map(h -> h.getUserMessage(err))
- .filter(Optional::isPresent)
- .map(Optional::get)
- .collect(toImmutableList());
-
- Optional<Integer> statusCode =
- globals.exceptionHooks.stream()
- .map(h -> h.getStatusCode(err))
- .filter(Optional::isPresent)
- .map(Optional::get)
- .findFirst();
- if (statusCode.isPresent() && statusCode.get() < 400) {
- StringBuilder msg = new StringBuilder();
- if (userMessages.size() == 1) {
- msg.append(userMessages.get(0));
- } else {
- userMessages.forEach(m -> msg.append("\n* ").append(m));
- }
-
- res.setStatus(statusCode.get());
- logger.atFinest().withCause(err).log("REST call finished: %d", statusCode.get().intValue());
+ if (status.statusCode() < SC_BAD_REQUEST) {
+ logger.atFinest().withCause(err).log("REST call finished: %d", status.statusCode());
return replyText(req, res, true, msg.toString());
}
+ if (status.statusCode() >= SC_INTERNAL_SERVER_ERROR) {
+ logger.atSevere().withCause(err).log("Error in %s %s", req.getMethod(), uriForLogging(req));
+ }
+ return replyError(req, res, status.statusCode(), msg.toString(), err);
+ }
+ private static long replyInternalServerError(
+ HttpServletRequest req,
+ HttpServletResponse res,
+ Throwable err,
+ ImmutableList<String> userMessages)
+ throws IOException {
logger.atSevere().withCause(err).log("Error in %s %s", req.getMethod(), uriForLogging(req));
StringBuilder msg = new StringBuilder("Internal server error");
if (!userMessages.isEmpty()) {
+ msg.append("\n");
userMessages.forEach(m -> msg.append("\n* ").append(m));
}
- return replyError(req, res, statusCode.orElse(SC_INTERNAL_SERVER_ERROR), msg.toString(), err);
+
+ return replyError(req, res, SC_INTERNAL_SERVER_ERROR, msg.toString(), err);
}
private static String uriForLogging(HttpServletRequest req) {
diff --git a/java/com/google/gerrit/server/ExceptionHook.java b/java/com/google/gerrit/server/ExceptionHook.java
index 7a1bdde..89a0ab7 100644
--- a/java/com/google/gerrit/server/ExceptionHook.java
+++ b/java/com/google/gerrit/server/ExceptionHook.java
@@ -14,6 +14,11 @@
package com.google.gerrit.server;
+import static java.util.Objects.requireNonNull;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.annotations.ExtensionPoint;
import java.util.Optional;
@@ -40,6 +45,9 @@
* <p>If {@code false} is returned the operation is still retried once to capture a trace, unless
* {@link #skipRetryWithTrace(String, String, Throwable)} skips the auto-retry.
*
+ * <p>If multiple exception hooks are registered, the operation is retried if any of them returns
+ * {@code true} from this method.
+ *
* @param throwable throwable that was thrown while executing the operation
* @param actionType the type of the action for which the exception occurred
* @param actionName the name of the action for which the exception occurred
@@ -69,6 +77,9 @@
* <p>This method is only invoked if retry with tracing is enabled on the server ({@code
* retry.retryWithTraceOnFailure} in {@code gerrit.config} is set to {@code true}).
*
+ * <p>If multiple exception hooks are registered, retrying with tracing is skipped if any of them
+ * returns {@code true} from this method.
+ *
* @param throwable throwable that was thrown while executing the operation
* @param actionType the type of the action for which the exception occurred
* @param actionName the name of the action for which the exception occurred
@@ -85,6 +96,9 @@
* <p>This method allows implementors to group exceptions that have the same cause into one metric
* bucket.
*
+ * <p>If multiple exception hooks return a value from this method, the value from the exception
+ * hook that is registered first is used.
+ *
* @param throwable the exception cause
* @return formatted cause or {@link Optional#empty()} if no formatting was done
*/
@@ -93,36 +107,56 @@
}
/**
- * Returns a message that should be returned to the user.
+ * Returns messages that should be returned to the user.
*
- * <p>This message is included into the HTTP response that is sent to the user.
+ * <p>These messages are included into the HTTP response that is sent to the user.
+ *
+ * <p>If multiple exception hooks return a value from this method, all the values are included
+ * into the HTTP response (in the order in which the exception hooks are registered).
*
* @param throwable throwable that was thrown while executing an operation
- * @return error message that should be returned to the user, {@link Optional#empty()} if no
+ * @param traceId ID of the trace if this request was traced, otherwise {@code null}
+ * @return error messages that should be returned to the user, {@link Optional#empty()} if no
* message should be returned to the user
*/
- default Optional<String> getUserMessage(Throwable throwable) {
- return Optional.empty();
+ default ImmutableList<String> getUserMessages(Throwable throwable, @Nullable String traceId) {
+ return ImmutableList.of();
}
/**
- * Returns the HTTP status code that should be returned to the user.
+ * Returns the HTTP status that should be returned to the user.
*
- * <p>If no value is returned ({@link Optional#empty()}) the HTTP status code defaults to {@code
- * 500 Internal Server Error}.
+ * <p>Implementors may use this method to change the status for certain exceptions (e.g. using
+ * this method it would be possible to return {@code 503 Lock failure} for {@link
+ * com.google.gerrit.git.LockFailureException}s instead of {@code 500 Internal server error}).
*
- * <p>{@link #getUserMessage(Throwable)} allows to define which message should be included into
- * the body of the HTTP response.
+ * <p>If no value is returned ({@link Optional#empty()}) it means that this exception hook doesn't
+ * want to change the default response code for the given exception which is {@code 500 Internal
+ * Server Error}, but is fine if other exception hook implementation do so.
*
- * <p>Implementors may use this method to change the status code for certain exceptions (e.g.
- * using this method it would be possible to return {@code 409 Conflict} for {@link
- * com.google.gerrit.git.LockFailureException}s instead of {@code 500 Internal Server Error}).
+ * <p>If multiple exception hooks return a value from this method, the value from exception hook
+ * that is registered first is used.
+ *
+ * <p>{@link #getUserMessages(Throwable, String)} allows to define which additional messages
+ * should be included into the body of the HTTP response.
*
* @param throwable throwable that was thrown while executing an operation
- * @return HTTP status code that should be returned to the user, {@link Optional#empty()} if the
+ * @return HTTP status that should be returned to the user, {@link Optional#empty()} if the
* exception should result in {@code 500 Internal Server Error}
*/
- default Optional<Integer> getStatusCode(Throwable throwable) {
+ default Optional<Status> getStatus(Throwable throwable) {
return Optional.empty();
}
+
+ @AutoValue
+ public abstract class Status {
+ public abstract int statusCode();
+
+ public abstract String statusMessage();
+
+ public static Status create(int statusCode, String statusMessage) {
+ return new AutoValue_ExceptionHook_Status(
+ statusCode, requireNonNull(statusMessage, "statusMessage"));
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/ExceptionHookImpl.java b/java/com/google/gerrit/server/ExceptionHookImpl.java
index 0cc2545..8393451 100644
--- a/java/com/google/gerrit/server/ExceptionHookImpl.java
+++ b/java/com/google/gerrit/server/ExceptionHookImpl.java
@@ -16,6 +16,8 @@
import com.google.common.base.Predicate;
import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.git.LockFailureException;
import java.util.Optional;
import org.eclipse.jgit.lib.RefUpdate;
@@ -44,14 +46,22 @@
}
@Override
- public Optional<String> getUserMessage(Throwable throwable) {
+ public ImmutableList<String> getUserMessages(Throwable throwable, @Nullable String traceId) {
if (isLockFailure(throwable)) {
- return Optional.of(LOCK_FAILURE_USER_MESSAGE);
+ return ImmutableList.of(LOCK_FAILURE_USER_MESSAGE);
+ }
+ return ImmutableList.of();
+ }
+
+ @Override
+ public Optional<Status> getStatus(Throwable throwable) {
+ if (isLockFailure(throwable)) {
+ return Optional.of(Status.create(503, "Lock failure"));
}
return Optional.empty();
}
- public static boolean isLockFailure(Throwable throwable) {
+ private static boolean isLockFailure(Throwable throwable) {
return isMatching(throwable, t -> t instanceof LockFailureException);
}
