From 7a04708c41f54fe6f2307e3bd607b9bb7c103090 Mon Sep 17 00:00:00 2001
From: Andy Wilkinson <awilkinson@pivotal.io>
Date: Wed, 21 Jun 2017 14:19:10 -0700
Subject: [PATCH] Make sure the HealthMvcEndpoint is thread-safe

Previously, HealthMvcEndpoint stored the cached Health and its last
access time in two separate fields. Neither field was volatile and
no synchronization was used. This meant that there were potential
visibility problems. In a possible worst case scenario one field may
see the updated access time but an old health so it would incorrectly
believe that the old health was up-to-date and return it.

This commit reworks the endpoint to store the cached health and the
time at which it was created in a single, volatile field. This ensures
that the cached health and its creation time will be visible across
threads. Note that a race between threads when the cache is stale is
still possible. This race may result in multiple calls to the
delegate but these should be harmless.

Closes gh-9454
---
 .../endpoint/mvc/HealthMvcEndpoint.java       | 50 +++++++++++++------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java
index 9ebaf80d24e..48b73d2a817 100644
--- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java
+++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java
@@ -60,14 +60,12 @@ public class HealthMvcEndpoint extends AbstractEndpointMvcAdapter<HealthEndpoint
 
 	private final List<String> roles;
 
+	private volatile CachedHealth cachedHealth;
+
 	private Map<String, HttpStatus> statusMapping = new HashMap<String, HttpStatus>();
 
 	private RelaxedPropertyResolver securityPropertyResolver;
 
-	private long lastAccess = 0;
-
-	private Health cached;
-
 	public HealthMvcEndpoint(HealthEndpoint delegate) {
 		this(delegate, true);
 	}
@@ -165,22 +163,29 @@ public class HealthMvcEndpoint extends AbstractEndpointMvcAdapter<HealthEndpoint
 	}
 
 	private Health getHealth(HttpServletRequest request, Principal principal) {
-		long accessTime = System.currentTimeMillis();
-		if (isCacheStale(accessTime)) {
-			this.lastAccess = accessTime;
-			this.cached = getDelegate().invoke();
-		}
+		Health currentHealth = getCurrentHealth();
 		if (exposeHealthDetails(request, principal)) {
-			return this.cached;
+			return this.cachedHealth.health;
+		}
+		return Health.status(currentHealth.getStatus()).build();
+	}
+
+	private Health getCurrentHealth() {
+		long accessTime = System.currentTimeMillis();
+		CachedHealth cachedHealth = this.cachedHealth;
+		if (isStale(cachedHealth, accessTime)) {
+			Health health = getDelegate().invoke();
+			this.cachedHealth = new CachedHealth(health, accessTime);
+			return health;
 		}
-		return Health.status(this.cached.getStatus()).build();
+		return cachedHealth.health;
 	}
 
-	private boolean isCacheStale(long accessTime) {
-		if (this.cached == null) {
+	private boolean isStale(CachedHealth cachedHealth, long accessTime) {
+		if (cachedHealth == null) {
 			return true;
 		}
-		return (accessTime - this.lastAccess) >= getDelegate().getTimeToLive();
+		return (accessTime - cachedHealth.creationTime) >= getDelegate().getTimeToLive();
 	}
 
 	protected boolean exposeHealthDetails(HttpServletRequest request,
@@ -221,4 +226,21 @@ public class HealthMvcEndpoint extends AbstractEndpointMvcAdapter<HealthEndpoint
 				null) && principal instanceof Authentication;
 	}
 
+	/**
+	 * A cached {@link Health} that encapsulates the {@code Health} itself and the time at
+	 * which it was created.
+	 */
+	static class CachedHealth {
+
+		private final Health health;
+
+		private final long creationTime;
+
+		CachedHealth(Health health, long creationTime) {
+			this.health = health;
+			this.creationTime = creationTime;
+		}
+
+	}
+
 }