opendev/gerrit
Code Issues Proposed changes

RestApiServlet: Avoid using local field for trace ID as the instance may be reused

Browse Source 
It seems that instances of RestApiServlet can be reused for multiple
requests hence we must not use a local member field to store the trace
ID, since otherwise the trace ID of one request may be applied to
another request.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I83b667551fce85479bb9feab1a90f5ef69f14fdd
This commit is contained in:
Edwin Kempin
2019-11-27 13:32:52 +01:00
parent 00a77ac45e
commit 80477712f7
1 changed files with 11 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
java/com/google/gerrit/httpd/restapi/RestApiServlet.java
View File

@@ -175,6 +175,7 @@ import java.util.Optional;
import java.util.Set; import java.util.Set;
import java.util.TreeMap; import java.util.TreeMap;
import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;
import java.util.regex.Pattern; import java.util.regex.Pattern;
import java.util.stream.Stream; import java.util.stream.Stream;
import java.util.zip.GZIPOutputStream; import java.util.zip.GZIPOutputStream;
@@ -291,7 +292,6 @@ public class RestApiServlet extends HttpServlet {
private final Globals globals; private final Globals globals;
private final Provider<RestCollection<RestResource, RestResource>> members; private final Provider<RestCollection<RestResource, RestResource>> members;
private Optional<String> traceId = Optional.empty();
public RestApiServlet( public RestApiServlet(
Globals globals, RestCollection<? extends RestResource, ? extends RestResource> members) { Globals globals, RestCollection<? extends RestResource, ? extends RestResource> members) {
@@ -666,7 +666,7 @@ public class RestApiServlet extends HttpServlet {
responseBytes = replyError(req, res, status = SC_SERVICE_UNAVAILABLE, "Lock failure", e); responseBytes = replyError(req, res, status = SC_SERVICE_UNAVAILABLE, "Lock failure", e);
} else { } else {
status = SC_INTERNAL_SERVER_ERROR; status = SC_INTERNAL_SERVER_ERROR;
responseBytes = handleException(e, req, res); responseBytes = handleException(traceContext, e, req, res);
} }
} catch (QuotaException e) { } catch (QuotaException e) {
cause = Optional.of(e); cause = Optional.of(e);
@@ -681,7 +681,7 @@ public class RestApiServlet extends HttpServlet {
} catch (Exception e) { } catch (Exception e) {
cause = Optional.of(e); cause = Optional.of(e);
status = SC_INTERNAL_SERVER_ERROR; status = SC_INTERNAL_SERVER_ERROR;
responseBytes = handleException(e, req, res); responseBytes = handleException(traceContext, e, req, res);
} finally { } finally {
String metric = getViewName(viewData); String metric = getViewName(viewData);
String formattedCause = cause.map(globals.retryHelper::formatCause).orElse("_none"); String formattedCause = cause.map(globals.retryHelper::formatCause).orElse("_none");
@@ -814,6 +814,7 @@ public class RestApiServlet extends HttpServlet {
ActionType actionType, ActionType actionType,
Action<T> action) Action<T> action)
throws Exception { throws Exception {
AtomicReference<Optional<String>> traceId = new AtomicReference<>(Optional.empty());
RetryHelper.Options.Builder retryOptionsBuilder = RetryHelper.options().caller(caller); RetryHelper.Options.Builder retryOptionsBuilder = RetryHelper.options().caller(caller);
if (!traceContext.isTracing()) { if (!traceContext.isTracing()) {
// enable automatic retry with tracing in case of non-recoverable failure // enable automatic retry with tracing in case of non-recoverable failure
@@ -822,7 +823,7 @@ public class RestApiServlet extends HttpServlet {
.retryWithTrace(t -> !(t instanceof RestApiException)) .retryWithTrace(t -> !(t instanceof RestApiException))
.onAutoTrace( .onAutoTrace(
autoTraceId -> { autoTraceId -> {
traceId = Optional.of(autoTraceId); traceId.set(Optional.of(autoTraceId));
// Include details of the request into the trace. // Include details of the request into the trace.
traceRequestData(req); traceRequestData(req);
@@ -838,7 +839,9 @@ public class RestApiServlet extends HttpServlet {
// If auto-tracing got triggered due to a non-recoverable failure, also trace the rest of // 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 // this request. This means logging is forced for all further log statements and the logs are
// associated with the same trace ID. // associated with the same trace ID.
traceId.ifPresent(tid -> traceContext.addTag(RequestId.Type.TRACE_ID, tid).forceLogging()); traceId
.get()
.ifPresent(tid -> traceContext.addTag(RequestId.Type.TRACE_ID, tid).forceLogging());
} }
} }
@@ -1660,12 +1663,13 @@ public class RestApiServlet extends HttpServlet {
} }
} }
private long handleException(Throwable err, HttpServletRequest req, HttpServletResponse res) private long handleException(
TraceContext traceContext, Throwable err, HttpServletRequest req, HttpServletResponse res)
throws IOException { throws IOException {
logger.atSevere().withCause(err).log("Error in %s %s", req.getMethod(), uriForLogging(req)); logger.atSevere().withCause(err).log("Error in %s %s", req.getMethod(), uriForLogging(req));
if (!res.isCommitted()) { if (!res.isCommitted()) {
res.reset(); res.reset();
traceId.ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId)); traceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
StringBuilder msg = new StringBuilder("Internal server error"); StringBuilder msg = new StringBuilder("Internal server error");
ImmutableList<String> userMessages = ImmutableList<String> userMessages =
globals.exceptionHooks.stream() globals.exceptionHooks.stream()