diff --git a/changelog/unreleased/SOLR-18531-jetty-runner-port-reservation.yml b/changelog/unreleased/SOLR-18531-jetty-runner-port-reservation.yml new file mode 100644 index 000000000000..75d6f9657724 --- /dev/null +++ b/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 diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java b/solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java index f9acdbfc0bf5..75429b1ccc37 100644 --- a/solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java +++ b/solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java @@ -624,8 +624,9 @@ public void shutdown() throws Exception { }); solrClientByCollection.clear(); - List> shutdowns = new ArrayList<>(jettys.size()); - for (final JettySolrRunner jetty : jettys) { + List stoppedJettys = new ArrayList<>(jettys); + List> 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; } diff --git a/solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java b/solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java index ff8253853ff1..9b3b0db7cbc0 100644 --- a/solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java +++ b/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 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(); } } diff --git a/solr/test-framework/src/test/org/apache/solr/embedded/TestJettySolrRunner.java b/solr/test-framework/src/test/org/apache/solr/embedded/TestJettySolrRunner.java index a874787c0ead..ce5707c0d134 100644 --- a/solr/test-framework/src/test/org/apache/solr/embedded/TestJettySolrRunner.java +++ b/solr/test-framework/src/test/org/apache/solr/embedded/TestJettySolrRunner.java @@ -18,6 +18,8 @@ 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; @@ -25,7 +27,9 @@ 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(); + } + } + } }