From 9e81fb3e38db217f6dd08d246784d88914e5637f Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Fri, 23 Jul 2021 10:25:38 +0100 Subject: [PATCH] Fix race condition in Jetty graceful shutdown tests Some of the Jetty graceful shutdown tests were flaky due to the way in which Jetty behaves when it is stopped. Stopping the Jetty web server interrupts the thread that's handling the active request. This initiates a race between the request-handling thread which will decrement the number of active requests and the main thread which expects an active request to cause the shutdown result to be REQUESTS_ACTIVE. The test passes when the main thread wins and fails as a request is active which it's checked. When the request-handling thread wins the test fails as the count of active requests has been deprecated before it is checked. The blocking servlet that's used to stall a request and keep it active needs to be updated to ignore the thread being interrupted and continue waiting. This will ensure that a request remains active until the main thread has checked the active request count and determine the result of the shutdown. Closes gh-27464 --- .../AbstractServletWebServerFactoryTests.java | 100 ++++++++---------- 1 file changed, 47 insertions(+), 53 deletions(-) diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java index d365487143f..40f128cb982 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java @@ -44,11 +44,11 @@ import java.util.EnumSet; import java.util.HashMap; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.BlockingQueue; import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.Callable; -import java.util.concurrent.CyclicBarrier; import java.util.concurrent.Future; import java.util.concurrent.FutureTask; import java.util.concurrent.RunnableFuture; @@ -1136,8 +1136,8 @@ public abstract class AbstractServletWebServerFactoryTests { AtomicReference result = new AtomicReference<>(); this.webServer.shutDownGracefully(result::set); this.webServer.stop(); - Awaitility.await().atMost(Duration.ofSeconds(30)) - .until(() -> GracefulShutdownResult.REQUESTS_ACTIVE == result.get()); + assertThat(Awaitility.await().atMost(Duration.ofSeconds(30)).until(result::get, Objects::nonNull)) + .isEqualTo(GracefulShutdownResult.REQUESTS_ACTIVE); try { blockingServlet.admitOne(); } @@ -1486,7 +1486,7 @@ public abstract class AbstractServletWebServerFactoryTests { protected static class BlockingServlet extends HttpServlet { - private final BlockingQueue barriers = new ArrayBlockingQueue<>(10); + private final BlockingQueue blockers = new ArrayBlockingQueue<>(10); public BlockingServlet() { @@ -1494,42 +1494,23 @@ public abstract class AbstractServletWebServerFactoryTests { @Override protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { - CyclicBarrier barrier = new CyclicBarrier(2); - this.barriers.add(barrier); - try { - barrier.await(); - } - catch (InterruptedException ex) { - Thread.currentThread().interrupt(); - } - catch (BrokenBarrierException ex) { - throw new ServletException(ex); - } + Blocker blocker = new Blocker(); + this.blockers.add(blocker); + blocker.await(); } - public void admitOne() { - try { - CyclicBarrier barrier = this.barriers.take(); - if (!barrier.isBroken()) { - barrier.await(); - } - } - catch (InterruptedException ex) { - Thread.currentThread().interrupt(); - } - catch (BrokenBarrierException ex) { - throw new RuntimeException(ex); - } + public void admitOne() throws InterruptedException { + this.blockers.take().clear(); } public void awaitQueue() throws InterruptedException { - while (this.barriers.isEmpty()) { + while (this.blockers.isEmpty()) { Thread.sleep(100); } } public void awaitQueue(int size) throws InterruptedException { - while (this.barriers.size() < size) { + while (this.blockers.size() < size) { Thread.sleep(100); } } @@ -1538,45 +1519,58 @@ public abstract class AbstractServletWebServerFactoryTests { static class BlockingAsyncServlet extends HttpServlet { - private final BlockingQueue barriers = new ArrayBlockingQueue<>(10); + private final BlockingQueue blockers = new ArrayBlockingQueue<>(10); @Override protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { - CyclicBarrier barrier = new CyclicBarrier(2); - this.barriers.add(barrier); + Blocker blocker = new Blocker(); + this.blockers.add(blocker); AsyncContext async = req.startAsync(); new Thread(() -> { - try { - barrier.await(); - } - catch (InterruptedException ex) { - Thread.currentThread().interrupt(); - } - catch (BrokenBarrierException ex) { - - } + blocker.await(); async.complete(); }).start(); } - private void admitOne() { - try { - this.barriers.take().await(); - } - catch (InterruptedException ex) { - Thread.currentThread().interrupt(); - } - catch (BrokenBarrierException ex) { - throw new RuntimeException(ex); - } + private void admitOne() throws InterruptedException { + this.blockers.take().clear(); } private void awaitQueue() throws InterruptedException { - while (this.barriers.isEmpty()) { + while (this.blockers.isEmpty()) { Thread.sleep(100); } } } + private static final class Blocker { + + private boolean block = true; + + private final Object monitor = new Object(); + + private void await() { + synchronized (this.monitor) { + while (this.block) { + try { + this.monitor.wait(); + } + catch (InterruptedException ex) { + System.out.println("Interrupted!"); + // Keep waiting + } + } + } + } + + private void clear() { + synchronized (this.monitor) { + this.block = false; + this.monitor.notifyAll(); + } + } + + } + }