Make GlobalHealthCheck reentrant
The GlobalHealthCheck is called by multiple threads
and should never be stored in the instance variable
of a singleton.
The only exception is for the shallow storage of the
latest result that is invoked by the metrics collector,
which is well known to be subject to erasure due
to concurrency and may not be 100% accurate.
Change-Id: I17b3913274d83cf29a51a521a26b3c50d60b4658
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java
index fb298fd..cea2530 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java
@@ -15,6 +15,7 @@
package com.googlesource.gerrit.plugins.healthcheck;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.Sets;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
@@ -54,7 +55,10 @@
@Override
public void start() {
- for (HealthCheck healthCheck : healthChecks) {
+ Set<HealthCheck> allChecks = Sets.newHashSet(healthChecks);
+ allChecks.add(globalHealthCheck);
+
+ for (HealthCheck healthCheck : allChecks) {
String name = healthCheck.name();
Counter0 failureMetric = getFailureMetric(name);
@@ -65,21 +69,6 @@
registeredMetrics.add(metricMaker.newTrigger(latencyMetric, metricCallBack));
triggers.add(metricCallBack);
}
-
- Counter0 globalFailureMetric = getFailureMetric("global");
- CallbackMetric0<Long> globalLatencyMetric = getLatencyMetric("global");
- Runnable globalMetricCallBack =
- () -> {
- HealthCheck.StatusSummary status = globalHealthCheck.getGlobalStatusSummary();
- globalLatencyMetric.set(status.elapsed);
- if (status.isFailure()) {
- globalFailureMetric.increment();
- }
- };
-
- registeredMetrics.add(globalFailureMetric);
- registeredMetrics.add(metricMaker.newTrigger(globalLatencyMetric, globalMetricCallBack));
- triggers.add(globalMetricCallBack);
}
private Runnable getCallbackMetric(
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java
index 9e0a607..2d5f7de 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java
@@ -20,6 +20,7 @@
import com.google.inject.Singleton;
import com.googlesource.gerrit.plugins.healthcheck.check.GlobalHealthCheck;
import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck;
+import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.Result;
import java.util.Map;
import javax.servlet.http.HttpServletResponse;
@@ -36,15 +37,16 @@
@Override
public Response<Map<String, Object>> apply(ConfigResource resource)
throws AuthException, BadRequestException, ResourceConflictException, Exception {
- Map<String, Object> result = healthChecks.run();
+ HealthCheck.StatusSummary globalHealthCheckStatus = healthChecks.run();
- result.put("ts", healthChecks.getGlobalStatusSummary().ts);
- result.put("elapsed", healthChecks.getGlobalStatusSummary().elapsed);
- return Response.withStatusCode(getHTTPResultCode(result), result);
+ Map<String, Object> result = globalHealthCheckStatus.subChecks;
+ result.put("ts", globalHealthCheckStatus.ts);
+ result.put("elapsed", globalHealthCheckStatus.elapsed);
+ return Response.withStatusCode(getHTTPResultCode(globalHealthCheckStatus), result);
}
- private int getHTTPResultCode(Map<String, Object> result) {
- return healthChecks.getResultStatus(result) == HealthCheck.Result.FAILED
+ private int getHTTPResultCode(HealthCheck.StatusSummary checkStatus) {
+ return checkStatus.result == Result.FAILED
? HttpServletResponse.SC_INTERNAL_SERVER_ERROR
: HttpServletResponse.SC_OK;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java
index a6eae79..d0181ab 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java
@@ -17,6 +17,7 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig;
+import java.util.Collections;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
@@ -28,7 +29,7 @@
private final long timeout;
private final String name;
private final ListeningExecutorService executor;
- protected StatusSummary latestStatus;
+ protected volatile StatusSummary latestStatus;
protected HealthCheckConfig config;
protected AbstractHealthCheck(
@@ -47,6 +48,7 @@
@Override
public StatusSummary run() {
+ StatusSummary checkStatusSummary;
boolean enabled = config.healthCheckEnabled(name);
final long ts = System.currentTimeMillis();
ListenableFuture<StatusSummary> resultFuture =
@@ -59,18 +61,24 @@
log.warn("Check {} failed", name, e);
healthy = Result.FAILED;
}
- return new StatusSummary(healthy, ts, System.currentTimeMillis() - ts);
+ return new StatusSummary(
+ healthy, ts, System.currentTimeMillis() - ts, Collections.emptyMap());
});
try {
- latestStatus = resultFuture.get(timeout, TimeUnit.MILLISECONDS);
+ checkStatusSummary = resultFuture.get(timeout, TimeUnit.MILLISECONDS);
} catch (TimeoutException e) {
log.warn("Check {} timed out", name, e);
- latestStatus = new StatusSummary(Result.TIMEOUT, ts, System.currentTimeMillis() - ts);
+ checkStatusSummary =
+ new StatusSummary(
+ Result.TIMEOUT, ts, System.currentTimeMillis() - ts, Collections.emptyMap());
} catch (InterruptedException | ExecutionException e) {
log.warn("Check {} failed while waiting for its future result", name, e);
- latestStatus = new StatusSummary(Result.FAILED, ts, System.currentTimeMillis() - ts);
+ checkStatusSummary =
+ new StatusSummary(
+ Result.FAILED, ts, System.currentTimeMillis() - ts, Collections.emptyMap());
}
- return latestStatus;
+ latestStatus = checkStatusSummary.shallowCopy();
+ return checkStatusSummary;
}
protected abstract Result doCheck() throws Exception;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java
index c9066c5..c464a9e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java
@@ -23,18 +23,18 @@
import java.util.stream.StreamSupport;
@Singleton
-public class GlobalHealthCheck {
+public class GlobalHealthCheck implements HealthCheck {
private final DynamicSet<HealthCheck> healthChecks;
- private HealthCheck.StatusSummary globalStatusSummary;
+ private volatile StatusSummary latestStatus = StatusSummary.INITIAL_STATUS;
@Inject
public GlobalHealthCheck(DynamicSet<HealthCheck> healthChecks) {
this.healthChecks = healthChecks;
- this.globalStatusSummary = HealthCheck.StatusSummary.INITIAL_STATUS;
}
- public Map<String, Object> run() {
+ @Override
+ public HealthCheck.StatusSummary run() {
Iterable<HealthCheck> iterable = () -> healthChecks.iterator();
long ts = System.currentTimeMillis();
Map<String, Object> checkToResults =
@@ -42,25 +42,33 @@
.map(check -> Arrays.asList(check.name(), check.run()))
.collect(Collectors.toMap(k -> (String) k.get(0), v -> v.get(1)));
long elapsed = System.currentTimeMillis() - ts;
- globalStatusSummary =
- new HealthCheck.StatusSummary(getResultStatus(checkToResults), ts, elapsed);
- return checkToResults;
+ StatusSummary globalStatus =
+ new HealthCheck.StatusSummary(
+ hasAnyFailureOnResults(checkToResults) ? Result.FAILED : Result.PASSED,
+ ts,
+ elapsed,
+ checkToResults);
+ latestStatus = globalStatus.shallowCopy();
+ return globalStatus;
}
- public HealthCheck.StatusSummary getGlobalStatusSummary() {
- return this.globalStatusSummary;
- }
-
- public HealthCheck.Result getResultStatus(Map<String, Object> result) {
- if (result.values().stream()
+ public static boolean hasAnyFailureOnResults(Map<String, Object> results) {
+ return results.values().stream()
.filter(
res ->
res instanceof HealthCheck.StatusSummary
&& ((HealthCheck.StatusSummary) res).isFailure())
- .findFirst()
- .isPresent()) {
- return HealthCheck.Result.FAILED;
- }
- return HealthCheck.Result.PASSED;
+ .findAny()
+ .isPresent();
+ }
+
+ @Override
+ public String name() {
+ return "global";
+ }
+
+ @Override
+ public StatusSummary getLatestStatus() {
+ return latestStatus;
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java
index de571d1..46acdde 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java
@@ -16,7 +16,9 @@
import com.google.gson.annotations.SerializedName;
import java.util.Arrays;
+import java.util.Collections;
import java.util.HashSet;
+import java.util.Map;
import java.util.Set;
public interface HealthCheck {
@@ -34,22 +36,29 @@
public class StatusSummary {
public static final StatusSummary INITIAL_STATUS =
- new StatusSummary(Result.PASSED, System.currentTimeMillis(), 0L);
+ new StatusSummary(Result.PASSED, System.currentTimeMillis(), 0L, Collections.emptyMap());
public final Result result;
public final long ts;
public final long elapsed;
+ public final transient Map<String, Object> subChecks;
+
public static final Set<Result> failingResults =
new HashSet<>(Arrays.asList(Result.FAILED, Result.TIMEOUT));
- public StatusSummary(Result result, long ts, long elapsed) {
+ public StatusSummary(Result result, long ts, long elapsed, Map<String, Object> subChecks) {
this.result = result;
this.ts = ts;
this.elapsed = elapsed;
+ this.subChecks = subChecks;
}
public Boolean isFailure() {
return failingResults.contains(this.result);
}
+
+ public StatusSummary shallowCopy() {
+ return new StatusSummary(result, ts, elapsed, Collections.emptyMap());
+ }
}
StatusSummary run();
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/GlobalHealthCheckTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/GlobalHealthCheckTest.java
deleted file mode 100644
index a1c792e..0000000
--- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/GlobalHealthCheckTest.java
+++ /dev/null
@@ -1,55 +0,0 @@
-// Copyright (C) 2021 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.googlesource.gerrit.plugins.healthcheck;
-
-import static com.google.common.truth.Truth.assertThat;
-
-import com.google.gerrit.extensions.registration.DynamicSet;
-import com.google.inject.Inject;
-import com.googlesource.gerrit.plugins.healthcheck.check.GlobalHealthCheck;
-import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck;
-import java.util.HashMap;
-import org.junit.Test;
-
-public class GlobalHealthCheckTest {
-
- @Inject private DynamicSet<HealthCheck> healthChecks;
-
- GlobalHealthCheck globalHealthCheck = new GlobalHealthCheck(healthChecks);
-
- @Test
- public void shouldFailIfOneCheckFails() {
-
- HashMap<String, Object> checks =
- new HashMap<String, Object>() {
- {
- put("failingCheck", new HealthCheck.StatusSummary(HealthCheck.Result.FAILED, 0L, 0L));
- put("passingCheck", new HealthCheck.StatusSummary(HealthCheck.Result.PASSED, 0L, 0L));
- }
- };
- assertThat(globalHealthCheck.getResultStatus(checks)).isEqualTo(HealthCheck.Result.FAILED);
- }
-
- @Test
- public void shouldPassIfAllChecksPass() {
- HashMap<String, Object> checks =
- new HashMap<String, Object>() {
- {
- put("passingCheck", new HealthCheck.StatusSummary(HealthCheck.Result.PASSED, 0L, 0L));
- }
- };
- assertThat(globalHealthCheck.getResultStatus(checks)).isEqualTo(HealthCheck.Result.PASSED);
- }
-}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java
index fb64048..1c01950 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java
@@ -33,6 +33,7 @@
import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck;
import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.Result;
import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.StatusSummary;
+import java.util.Collections;
import org.junit.Before;
import org.junit.Test;
@@ -69,7 +70,7 @@
@Test
public void shouldSendCounterWhenStatusSummaryFailed() {
Long elapsed = 100L;
- setWithStatusSummary(new StatusSummary(Result.FAILED, 1L, elapsed));
+ setWithStatusSummary(new StatusSummary(Result.FAILED, 1L, elapsed, Collections.emptyMap()));
assertThat(testMetrics.getFailures()).isEqualTo(1);
assertThat(testMetrics.getLatency()).isEqualTo(elapsed);
@@ -78,7 +79,7 @@
@Test
public void shouldSendCounterWhenStatusSummaryTimeout() {
Long elapsed = 100L;
- setWithStatusSummary(new StatusSummary(Result.TIMEOUT, 1L, elapsed));
+ setWithStatusSummary(new StatusSummary(Result.TIMEOUT, 1L, elapsed, Collections.emptyMap()));
assertThat(testMetrics.getFailures()).isEqualTo(1);
assertThat(testMetrics.getLatency()).isEqualTo(elapsed);
@@ -87,7 +88,7 @@
@Test
public void shouldNOTSendCounterWhenStatusSummarySuccess() {
Long elapsed = 100L;
- setWithStatusSummary(new StatusSummary(Result.PASSED, 1L, elapsed));
+ setWithStatusSummary(new StatusSummary(Result.PASSED, 1L, elapsed, Collections.emptyMap()));
assertThat(testMetrics.failures).isEqualTo(0L);
assertThat(testMetrics.getLatency()).isEqualTo(elapsed);
