feat: Address review and Sonar findings on stop draining

Author: thced

README.md

@@ -574,21 +574,23 @@ try (var server = OpenApiServer.builder()
 `stop(int)` and `shutdownTimeoutSeconds(int)` reject negative values with
 `IllegalArgumentException`.
 
-Stopping closes the listening socket at once, so no new connection is accepted. Requests on
-keep-alive connections that are already open are served until the JDK sees no exchange in flight,
-which can come before the after-response runnables of the last request have finished; the JDK then
-closes every connection, keep-alive ones included. The wait ends as soon as `inFlightRequests()` reads `0`, or when the delay elapses; every connection
-is then closed, which cuts off what is still running. A request counts as in flight from the first
-filter until its after-response runnables (`Request.afterResponse`, hooks) have run, so `stop` waits
-for those too. A request the JDK rejects before it dispatches it, such as a malformed one, is not
-counted.
+Stopping closes the listening socket at once, so no new connection is accepted.
+Requests on keep-alive connections that are already open are still served
+until the JDK sees no exchange in flight.
+The wait ends as soon as `inFlightRequests()` reads `0`, or when the delay elapses.
+Every connection is then closed, which cuts off what is still running.
+A request counts as in flight from the first filter
+until its after-response runnables (`Request.afterResponse`, hooks) have run,
+so `stop` waits for those too.
+A request the JDK rejects before it dispatches it, such as a malformed one, is not counted.
 
 Only the first `stop` call stops the server. A later or concurrent call waits until that stop is
 complete, then returns (or throws `IllegalStateException` if that stop failed); its own delay is
 ignored. A thread interrupted while it waits stops waiting and keeps its interrupt flag.
 
-`inFlightRequests()` is the server's own count, readable at any time, for example to log it or to
-wait for `0` before stopping.
+`inFlightRequests()` is a snapshot of the server's own count, readable at any time,
+for example for logs and metrics.
+To drain before shutdown, call `stop(int)`, which waits for the count itself.
 
 ## Interceptors and response decorators
 

src/main/java/com/retailsvc/http/OpenApiServer.java

@@ -232,10 +232,10 @@ public InetAddress bindAddress() {
   }
 
   /**
-   * The number of requests being handled now, from the first filter to the end of the
-   * after-response hooks. A request the JDK rejects before it dispatches it, such as a malformed
-   * one, is not counted. Reads {@code 0} once the server is idle, so a caller can wait for that
-   * before {@link #stop(int)}.
+   * A snapshot of the number of requests being handled now, from the first filter to the end of the
+   * after-response hooks, for logs and metrics. A request the JDK rejects before it dispatches it,
+   * such as a malformed one, is not counted. To drain before shutdown, call {@link #stop(int)},
+   * which waits for this count itself.
    */
   public int inFlightRequests() {
     return counter.inFlight();
@@ -269,23 +269,23 @@ public void stop(int delaySeconds) {
       try {
         drainAndStop(delaySeconds);
         stopped.complete(null);
-      } catch (RuntimeException | Error e) {
+      } catch (RuntimeException e) {
         stopped.completeExceptionally(e);
         throw e;
+      } finally {
+        // An Error leaves the future open; complete it so no later call waits forever.
+        if (!stopped.isDone()) {
+          stopped.completeExceptionally(new IllegalStateException("The stop was aborted"));
+        }
       }
     } else {
       awaitStopped();
     }
   }
 
   /**
-   * {@code HttpServer.stop(delay)} closes the listening socket at once and then waits on a latch
-   * that, in the JDK this was written against, can stay unreleased with nothing in flight. It runs
-   * on its own thread for the first, and {@code stop(0)} ends it: {@code ServerImpl.stop} is safe
-   * to call again while another call waits. Each call only sets {@code terminating}, posts another
-   * {@code StopRequested} event, closes the already closed socket and waits on the latch; its
-   * {@code finally} counts the latch down for every waiter and clears connections and timers, which
-   * tolerates repeats.
+   * The first call runs {@code HttpServer.stop(delay)} on a virtual thread, which closes the
+   * listener; {@code stop(0)} releases its latch.
    */
   private void drainAndStop(int delaySeconds) {
     Thread background = ofVirtual().name("http-stop").start(() -> httpServer.stop(delaySeconds));
@@ -294,14 +294,13 @@ private void drainAndStop(int delaySeconds) {
     boolean interrupted = Thread.interrupted();
     httpServer.stop(0);
     try {
-      background.join(Duration.ofSeconds(STOP_JOIN_SECONDS));
+      if (!background.join(Duration.ofSeconds(STOP_JOIN_SECONDS))) {
+        LOG.warn(
+            "The JDK stop thread is still running {} s after the server was stopped",
+            STOP_JOIN_SECONDS);
+      }
     } catch (InterruptedException _) {
-      interrupted = true;
-    }
-    if (background.isAlive()) {
-      LOG.warn(
-          "The JDK stop thread is still running {} s after the server was stopped",
-          STOP_JOIN_SECONDS);
+      Thread.currentThread().interrupt();
     }
     if (interrupted) {
       Thread.currentThread().interrupt();
@@ -314,7 +313,7 @@ private void awaitStopped() {
     } catch (InterruptedException _) {
       Thread.currentThread().interrupt();
     } catch (ExecutionException e) {
-      throw new IllegalStateException("The first stop failed", e.getCause());
+      throw new IllegalStateException("The first stop failed", e);
     }
   }
 

src/main/java/com/retailsvc/http/internal/InFlightCounter.java

@@ -62,27 +62,27 @@ public boolean awaitIdle(long timeout, TimeUnit unit) {
     return inFlight() == 0;
   }
 
-  private void enter() {
-    lock.lock();
-    try {
-      inFlight++;
-    } finally {
-      lock.unlock();
-    }
-  }
+  private final class CountingFilter extends Filter {
 
-  private void exit() {
-    lock.lock();
-    try {
-      if (--inFlight == 0) {
-        idle.signalAll();
+    private void enter() {
+      lock.lock();
+      try {
+        inFlight++;
+      } finally {
+        lock.unlock();
       }
-    } finally {
-      lock.unlock();
     }
-  }
 
-  private final class CountingFilter extends Filter {
+    private void exit() {
+      lock.lock();
+      try {
+        if (--inFlight == 0) {
+          idle.signalAll();
+        }
+      } finally {
+        lock.unlock();
+      }
+    }
 
     @Override
     public void doFilter(HttpExchange exchange, Chain chain) throws IOException {

src/test/java/com/retailsvc/http/GracefulStopIT.java

@@ -51,6 +51,7 @@ class GracefulStopIT {
   private static final int ROUNDS = 100;
   private static final long MAX_PARK_NANOS = 400_000;
   private static final long LONG_ENOUGH_MILLIS = 300;
+  private static final long POLL_MILLIS = 10;
 
   private final CountDownLatch release = new CountDownLatch(1);
   private final CountDownLatch entered = new CountDownLatch(1);
@@ -240,7 +241,7 @@ void keepsTheInterruptFlagOfAThreadInterruptedWhileStopping() throws Exception {
   }
 
   @Test
-  void ignoresASecondStop() {
+  void returnsAtOnceWhenAlreadyStopped() {
     server = start(Map.of());
 
     server.stop(0);
@@ -343,13 +344,13 @@ private static boolean isDone(CompletableFuture<?> future, long millis)
   }
 
   /** Polls {@code condition} for up to five seconds. */
-  private static boolean eventually(BooleanSupplier condition) throws InterruptedException {
+  private static boolean eventually(BooleanSupplier condition) {
     long deadline = System.nanoTime() + Duration.ofSeconds(5).toNanos();
     while (System.nanoTime() < deadline) {
       if (condition.getAsBoolean()) {
         return true;
       }
-      Thread.sleep(10);
+      LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(POLL_MILLIS));
     }
     return condition.getAsBoolean();
   }