From 81dc0cc3cf3f6075c93bb7a8f70cc622d8ab8bd6 Mon Sep 17 00:00:00 2001
From: Andy Wilkinson <andy.wilkinson@broadcom.com>
Date: Fri, 15 Mar 2024 15:26:32 +0000
Subject: [PATCH] Eliminate race condition in Tomcat's graceful shutdown

There was a race condition between the thread that's waiting for
Tomcat to become inactive or the graceful shutdown to be aborted
and the thread that aborts the shutdown and stops Tomcat when the
grace period has elapsed. This race can lead to Tomcat appearing
to have become inactive before the abort of the shutdown is
noticed. When this happens, the result of the shutdown is reported
as IDLE when it should have been REQUESTS_ACTIVE. The consequences
of this are mostly benign although it does affect the log messages
that are emitted. It is also causing some of our graceful shutdown
tests to be flaky.

This commit eliminates the race condition by considering the
state of the aborted flag before logging and returning the result
of the shutdown.

Closes gh-39942
---
 .../web/embedded/tomcat/GracefulShutdown.java | 41 +++++++++++--------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/GracefulShutdown.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/GracefulShutdown.java
index b94f92c6d3d..8448db3b3fd 100644
--- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/GracefulShutdown.java
+++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/GracefulShutdown.java
@@ -67,26 +67,15 @@ final class GracefulShutdown {
 			List<Connector> connectors = getConnectors();
 			connectors.forEach(this::close);
 			shutdownUnderway.countDown();
-			try {
-				for (Container host : this.tomcat.getEngine().findChildren()) {
-					for (Container context : host.findChildren()) {
-						while (isActive(context)) {
-							if (this.aborted) {
-								logger.info("Graceful shutdown aborted with one or more requests still active");
-								callback.shutdownComplete(GracefulShutdownResult.REQUESTS_ACTIVE);
-								return;
-							}
-							Thread.sleep(50);
-						}
-					}
-				}
-
+			awaitInactiveOrAborted();
+			if (this.aborted) {
+				logger.info("Graceful shutdown aborted with one or more requests still active");
+				callback.shutdownComplete(GracefulShutdownResult.REQUESTS_ACTIVE);
 			}
-			catch (InterruptedException ex) {
-				Thread.currentThread().interrupt();
+			else {
+				logger.info("Graceful shutdown complete");
+				callback.shutdownComplete(GracefulShutdownResult.IDLE);
 			}
-			logger.info("Graceful shutdown complete");
-			callback.shutdownComplete(GracefulShutdownResult.IDLE);
 		}
 		finally {
 			shutdownUnderway.countDown();
@@ -106,6 +95,22 @@ final class GracefulShutdown {
 		connector.getProtocolHandler().closeServerSocketGraceful();
 	}
 
+	private void awaitInactiveOrAborted() {
+		try {
+			for (Container host : this.tomcat.getEngine().findChildren()) {
+				for (Container context : host.findChildren()) {
+					while (!this.aborted && isActive(context)) {
+						Thread.sleep(50);
+					}
+				}
+			}
+
+		}
+		catch (InterruptedException ex) {
+			Thread.currentThread().interrupt();
+		}
+	}
+
 	private boolean isActive(Container context) {
 		try {
 			if (((StandardContext) context).getInProgressAsyncCount() > 0) {