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

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;
+		}
+
+	}
+
 }