SOLR-18531: Reserve a stopped test runner's port until its next start

changelog/unreleased/SOLR-18531-jetty-runner-port-reservation.yml

@@ -0,0 +1,9 @@
+title: >-
+  JettySolrRunner in the test framework now holds a stopped runner's port in reserve until
+  the next start on that port
+type: fixed
+authors:
+  - name: Nick Shanin
+links:
+  - name: SOLR-18531
+    url: https://issues.apache.org/jira/browse/SOLR-18531

solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java

@@ -624,8 +624,9 @@ public void shutdown() throws Exception {
               });
       solrClientByCollection.clear();
 
-      List<Callable<JettySolrRunner>> shutdowns = new ArrayList<>(jettys.size());
-      for (final JettySolrRunner jetty : jettys) {
+      List<JettySolrRunner> stoppedJettys = new ArrayList<>(jettys);
+      List<Callable<JettySolrRunner>> shutdowns = new ArrayList<>(stoppedJettys.size());
+      for (final JettySolrRunner jetty : stoppedJettys) {
         shutdowns.add(() -> stopJettySolrRunner(jetty));
       }
       jettys.clear();
@@ -635,6 +636,14 @@ public void shutdown() throws Exception {
       ExecutorUtil.shutdownAndAwaitTermination(executorCloser);
       Exception shutdownError =
           checkForExceptions("Error shutting down MiniSolrCloudCluster", futures);
+      // A stopped runner holds its port in reserve for a restart; this cluster will not
+      // restart its runners, so release the reservations instead of keeping the ports
+      // until the JVM exits. This covers only the runners still in the cluster: a runner
+      // removed earlier by stopJettySolrRunner is not in this list, and its reservation
+      // stays in place until that runner is closed or the JVM exits.
+      for (final JettySolrRunner jetty : stoppedJettys) {
+        jetty.releasePortReservation();
+      }
       if (shutdownError != null) {
         throw shutdownError;
       }

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

@@ -26,7 +26,9 @@
 import java.io.PrintStream;
 import java.lang.invoke.MethodHandles;
 import java.net.BindException;
+import java.net.InetSocketAddress;
 import java.net.MalformedURLException;
+import java.net.ServerSocket;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
@@ -41,6 +43,7 @@
 import java.util.Properties;
 import java.util.Random;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import org.apache.solr.SolrBackend;
@@ -109,6 +112,18 @@ public class JettySolrRunner implements SolrBackend {
   // NOTE: needs to be larger than SolrHttpClient.threadPoolSweeperMaxIdleTime
   private static final int THREAD_POOL_MAX_IDLE_TIME_MS = 260000;
 
+  // Ports held on behalf of stopped runners, so that no other process can take a stopped
+  // runner's port before a runner starts on it again. Static because a replacement runner
+  // can be created for the same port (SSLMigrationTest does this); the reservation has to
+  // be visible across instances. An entry is released in exactly three ways: a runner
+  // starts on the port; MiniSolrCloudCluster.shutdown() releases the runners still in the
+  // cluster at shutdown; or the runner holding the entry is closed. In every other case
+  // the entry is held until the JVM exits. That includes a runner stopped and removed
+  // from its cluster and never closed, and a runner restarted on a different port than
+  // the one it reserved: a release only ever names the port a runner starts on or the
+  // port it currently holds, never a port it held earlier.
+  private static final Map<Integer, ServerSocket> RESERVED_PORTS = new ConcurrentHashMap<>();
+
   private Server server;
 
   private volatile ServletHolder solrServlet;
@@ -497,6 +512,10 @@ public synchronized void start(boolean reusePort) throws Exception {
       int port = reusePort && jettyPort != -1 ? jettyPort : this.config.port;
       log.info("Start Jetty (configured port={}, binding port={})", this.config.port, port);
 
+      // Release the reservation on the port we are about to bind, whether this runner's
+      // stop() left it or another runner was stopped on the same port.
+      releasePortReservation(port);
+
       // if started before, make a new server
       if (startedBefore) {
         init(port);
@@ -660,6 +679,10 @@ public synchronized void stop() throws Exception {
         }
       } while (!server.isStopped());
 
+      // Hold the port until the next start on it, so that another process cannot take
+      // it in the gap and make a restart fail with BindException.
+      reserveJettyPort();
+
     } finally {
       if (prevContext != null) {
         MDC.setContextMap(prevContext);
@@ -669,6 +692,67 @@ public synchronized void stop() throws Exception {
     }
   }
 
+  /**
+   * Holds this runner's port after stop, by binding a socket to it, until the reservation is
+   * released; see {@link #RESERVED_PORTS} for the exact release rules. If the port cannot be held,
+   * a restart behaves as it did without reservations: it binds the port if it is still free, and
+   * the usual bind retry applies if it is not.
+   */
+  private void reserveJettyPort() {
+    if (jettyPort <= 0) {
+      return;
+    }
+    if (RESERVED_PORTS.containsKey(jettyPort)) {
+      // A reservation for this port is already held: a second stop() on this runner
+      // (close() after stop(), or a stop followed by cluster shutdown), or another
+      // runner stopped on the same port. Binding again would fail against the held
+      // socket and log a warning claiming the port is unprotected when it is not.
+      return;
+    }
+    ServerSocket socket = null;
+    try {
+      socket = new ServerSocket();
+      // Address reuse lets this bind succeed over connection sockets the stopped server
+      // left in TIME_WAIT (its connectors bind with reuse set as well); without it, a
+      // runner that served traffic before stopping could not reserve its port at all.
+      // The listening socket held here still refuses every later bind, with or without
+      // reuse, on Linux; address-reuse bind semantics differ on Windows, where the
+      // same exclusion is not verified.
+      socket.setReuseAddress(true);
+      socket.bind(new InetSocketAddress("127.0.0.1", jettyPort));
+    } catch (IOException e) {
+      log.warn(
+          "Could not reserve port {} after stop; a restart on this port is not protected",
+          jettyPort,
+          e);
+      IOUtils.closeQuietly(socket);
+      return;
+    }
+    if (RESERVED_PORTS.putIfAbsent(jettyPort, socket) != null) {
+      IOUtils.closeQuietly(socket);
+    }
+  }
+
+  /**
+   * Releases the reservation held on this runner's current port, if any. Called by
+   * MiniSolrCloudCluster.shutdown() for the runners still in the cluster at shutdown, whose runners
+   * will not start again, and by {@link #close()}, after which this runner will not start again
+   * either. A runner removed from its cluster by {@code stopJettySolrRunner} is not covered by the
+   * shutdown call: if it is never closed, its reservation stays in place until a runner starts on
+   * the port or the JVM exits. The same applies to the old port of a runner restarted on a
+   * different port, because this method names only the port the runner currently holds.
+   */
+  public void releasePortReservation() {
+    releasePortReservation(jettyPort);
+  }
+
+  private static void releasePortReservation(int port) {
+    ServerSocket socket = RESERVED_PORTS.remove(port);
+    if (socket != null) {
+      IOUtils.closeQuietly(socket);
+    }
+  }
+
   public void outputMetrics(PrintStream out) throws IOException {
     if (getCoreContainer() != null) {
       SolrMetricManager metricsManager = getCoreContainer().getMetricManager();
@@ -887,5 +971,10 @@ public void close() {
     } catch (Exception e) {
       log.error(e.toString(), e); // nowarn
     }
+    // close() is the end of this runner's life, unlike stop(), which is one half of the
+    // stop and restart cycle the reservation protects. Give the port back instead of
+    // leaving stop()'s reservation in place until the JVM exits. Runners still in a
+    // MiniSolrCloudCluster get the same release from its shutdown().
+    releasePortReservation();
   }
 }

solr/test-framework/src/test/org/apache/solr/embedded/TestJettySolrRunner.java

@@ -18,14 +18,18 @@
 
 import java.io.IOException;
 import java.net.BindException;
+import java.net.InetSocketAddress;
+import java.net.ServerSocket;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Properties;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.jetty.HttpJettySolrClient;
 import org.apache.solr.client.solrj.request.CoreAdminRequest;
+import org.apache.solr.client.solrj.request.CoresApi;
 import org.apache.solr.cloud.MiniSolrCloudCluster;
 import org.junit.Test;
 
@@ -113,4 +117,117 @@ public synchronized Throwable getCause() {
     result = jetty.lookForBindException(test);
     assertEquals(result, be);
   }
+
+  @Test
+  public void testStoppedRunnerKeepsItsPortUntilRestart() throws Exception {
+    Path solrHome = createTempDir();
+    Files.write(
+        solrHome.resolve("solr.xml"),
+        MiniSolrCloudCluster.DEFAULT_CLOUD_SOLR_XML.getBytes(Charset.defaultCharset()));
+
+    JettyConfig config = JettyConfig.builder().build();
+    JettySolrRunner runner = new JettySolrRunner(solrHome.toString(), config);
+
+    boolean running = false;
+    try {
+      runner.start();
+      running = true;
+      int port = runner.getLocalPort();
+
+      runner.stop();
+      running = false;
+
+      // The framework holds the stopped runner's port, so a foreign process cannot take
+      // it during the restart gap. This uses only the long-standing public API, so the
+      // test also runs against the pre-fix framework, where the bind below succeeds.
+      try (ServerSocket foreign = new ServerSocket()) {
+        foreign.setReuseAddress(false);
+        foreign.bind(new InetSocketAddress("127.0.0.1", port));
+        fail("the stopped runner's port should still be reserved");
+      } catch (BindException expected) {
+        // the reservation is doing its job
+      }
+
+      // Restarting on the same port still works.
+      runner.start();
+      running = true;
+      assertEquals(port, runner.getLocalPort());
+
+      // Closing a stopped runner gives the port back. close() ends the runner's life,
+      // so nothing keeps the reservation afterwards, unlike stop(), which is one half
+      // of the stop and restart cycle.
+      runner.stop();
+      running = false;
+      runner.close();
+      try (ServerSocket foreign = new ServerSocket()) {
+        foreign.setReuseAddress(false);
+        foreign.bind(new InetSocketAddress("127.0.0.1", port));
+        // the bind succeeded, so the reservation is gone
+      } catch (BindException e) {
+        fail("close() should release the stopped runner's port reservation");
+      }
+    } finally {
+      if (running) {
+        runner.stop();
+      }
+    }
+  }
+
+  @Test
+  public void testStoppedRunnerThatServedTrafficKeepsItsPortUntilRestart() throws Exception {
+    Path solrHome = createTempDir();
+    Files.write(
+        solrHome.resolve("solr.xml"),
+        MiniSolrCloudCluster.DEFAULT_CLOUD_SOLR_XML.getBytes(Charset.defaultCharset()));
+
+    JettyConfig config = JettyConfig.builder().build();
+    JettySolrRunner runner = new JettySolrRunner(solrHome.toString(), config);
+
+    boolean running = false;
+    try {
+      runner.start();
+      running = true;
+      int port = runner.getLocalPort();
+
+      // Serve a real request before the stop, through a client of this test rather than
+      // the runner's own, which stop() closes. Connections a node served can leave the
+      // port in TIME_WAIT once it stops, which the reservation bind has to tolerate (it
+      // binds with address reuse, like the server connectors) while still holding the
+      // port. The plain stop in the test above never serves traffic, so it cannot reach
+      // this case.
+      try (HttpJettySolrClient client =
+          new HttpJettySolrClient.Builder(runner.getBaseUrl().toString()).build()) {
+        new CoresApi.GetAllCoreStatus().process(client);
+      }
+
+      runner.stop();
+      running = false;
+
+      // A foreign bind with address reuse could take over connection sockets lingering
+      // in TIME_WAIT, but it must still fail against the socket the framework itself
+      // holds on the port, with or without reuse.
+      for (boolean reuse : new boolean[] {false, true}) {
+        try (ServerSocket foreign = new ServerSocket()) {
+          foreign.setReuseAddress(reuse);
+          foreign.bind(new InetSocketAddress("127.0.0.1", port));
+          fail("the stopped runner's port should still be reserved (reuseAddress=" + reuse + ")");
+        } catch (BindException expected) {
+          // the reservation is doing its job
+        }
+      }
+
+      // Restarting on the same port still works, and the runner serves again.
+      runner.start();
+      running = true;
+      assertEquals(port, runner.getLocalPort());
+      try (HttpJettySolrClient client =
+          new HttpJettySolrClient.Builder(runner.getBaseUrl().toString()).build()) {
+        new CoresApi.GetAllCoreStatus().process(client);
+      }
+    } finally {
+      if (running) {
+        runner.stop();
+      }
+    }
+  }
 }