SOLR-18336: bin/solr.cmd start should wait only for the started Solr instance

changelog/unreleased/SOLR-18336-windows-start-wait.yml

@@ -0,0 +1,12 @@
+title: >
+  Fixed bin/solr.cmd start on Windows waiting for all running Solr instances
+  instead of only the one being started. Also fixed the wait to connect using
+  the advertised or bind host instead of always localhost.
+type: fixed
+authors:
+  - name: Jan Høydahl
+    url: https://home.apache.org/phonebook.html?uid=janhoy
+  - name: Mikael Sterner
+links:
+  - name: SOLR-18336
+    url: https://issues.apache.org/jira/browse/SOLR-18336

solr/bin/solr.cmd

@@ -1178,7 +1178,7 @@ IF "%FG%"=="1" (
   "%JAVA%" %SOLR_SSL_OPTS% %AUTHC_OPTS% %SOLR_ZK_CREDS_AND_ACLS% %SOLR_TOOL_OPTS% -Dsolr.install.dir="%SOLR_TIP%" ^
     -Dlog4j.configurationFile="file:///%DEFAULT_SERVER_DIR%\resources\log4j2-console.xml" ^
     -classpath "%SOLR_TIP%\lib\*;%DEFAULT_SERVER_DIR%\solr-webapp\webapp\WEB-INF\lib\*;%DEFAULT_SERVER_DIR%\lib\ext\*" ^
-    org.apache.solr.cli.SolrCLI status --max-wait-secs !SOLR_START_WAIT!
+    org.apache.solr.cli.SolrCLI status -p %SOLR_PORT_LISTEN% --max-wait-secs !SOLR_START_WAIT!
   IF NOT "!ERRORLEVEL!"=="0" (
       set "SCRIPT_ERROR=Solr did not start or was not reachable. Check the logs for errors."
       goto err

solr/core/src/java/org/apache/solr/cli/SolrProcessManager.java

@@ -34,13 +34,16 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
+import java.util.concurrent.TimeUnit;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import org.apache.commons.io.IOUtils;
 import org.apache.lucene.util.Constants;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.util.EnvUtils;
+import org.apache.solr.common.util.TimeSource;
+import org.apache.solr.util.TimeOut;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -73,7 +76,14 @@ public SolrProcessManager() {
                     ProcessHandle::pid,
                     ph ->
                         new SolrProcess(
-                            ph.pid(), parsePortFromProcess(ph).orElseThrow(), isProcessSsl(ph))));
+                            ph.pid(),
+                            parseSyspropFromProcess(ph, "solr.port.listen")
+                                .map(Integer::parseInt)
+                                .orElseThrow(),
+                            isProcessSsl(ph),
+                            localConnectHost(
+                                parseSyspropFromProcess(ph, "solr.host.advertise"),
+                                parseSyspropFromProcess(ph, "solr.host.bind")))));
     portProcessMap =
         pidProcessMap.values().stream().collect(Collectors.toUnmodifiableMap(p -> p.port, p -> p));
     String solrInstallDir = EnvUtils.getProperty(SOLR_INSTALL_DIR);
@@ -143,13 +153,47 @@ public Collection<SolrProcess> getAllRunning() {
     return pidProcessMap.values();
   }
 
-  private Optional<Integer> parsePortFromProcess(ProcessHandle ph) {
-    Optional<String> portStr =
-        arguments(ph).stream()
-            .filter(a -> a.contains("-Dsolr.port.listen="))
-            .map(s -> s.split("=")[1])
-            .findFirst();
-    return portStr.isPresent() ? portStr.map(Integer::parseInt) : Optional.empty();
+  /** Parses the value of the given system property from the process' command line arguments */
+  private static Optional<String> parseSyspropFromProcess(ProcessHandle ph, String sysprop) {
+    return arguments(ph).stream()
+        .filter(a -> a.contains("-D" + sysprop + "="))
+        .map(s -> s.split("=", 2)[1])
+        .findFirst();
+  }
+
+  /**
+   * Returns the process listening on the given port, if found, waiting up to {@code maxWaitSecs}
+   * for it to appear. A newly started process may not be visible in the process table right away,
+   * so the table is re-scanned once a second until the deadline.
+   */
+  public Optional<SolrProcess> waitForProcessOnPort(int port, int maxWaitSecs)
+      throws InterruptedException {
+    Optional<SolrProcess> proc = processForPort(port);
+    TimeOut timeOut = new TimeOut(maxWaitSecs, TimeUnit.SECONDS, TimeSource.NANO_TIME);
+    while (proc.isEmpty() && !timeOut.hasTimedOut()) {
+      timeOut.sleep(1000);
+      proc = new SolrProcessManager().processForPort(port);
+    }
+    return proc;
+  }
+
+  /**
+   * Resolves the host to use when connecting locally to a Solr process. The advertised host is
+   * preferred when set, as that is the name the node is reachable by and, with SSL, the name its
+   * certificate is issued for. Otherwise the bind host is used if it is a specific non-loopback
+   * address. Wildcard and loopback binds are reachable as {@code localhost}. IPv6 literals are
+   * bracketed for use in URLs.
+   */
+  static String localConnectHost(Optional<String> advertiseHost, Optional<String> bindHost) {
+    String host =
+        advertiseHost
+            .map(String::trim)
+            .filter(h -> !h.isEmpty())
+            .orElseGet(() -> bindHost.map(String::trim).orElse(""));
+    return switch (host) {
+      case "", "0.0.0.0", "::", "[::]", "127.0.0.1", "::1", "[::1]", "localhost" -> "localhost";
+      default -> host.contains(":") && !host.startsWith("[") ? "[" + host + "]" : host;
+    };
   }
 
   private boolean isProcessSsl(ProcessHandle ph) {
@@ -237,11 +281,15 @@ private static List<String> arguments(ProcessHandle ph) {
     }
   }
 
-  /** Represents a running Solr process */
-  public record SolrProcess(long pid, int port, boolean isHttps) {
+  /**
+   * Represents a running Solr process. The {@code host} is the host to use when connecting to the
+   * process from the local machine, i.e. the advertised host if set, else the bind host if bound to
+   * a specific address, else {@code localhost}.
+   */
+  public record SolrProcess(long pid, int port, boolean isHttps, String host) {
 
     public String getLocalUrl() {
-      return String.format(Locale.ROOT, "%s://localhost:%s/solr", isHttps ? "https" : "http", port);
+      return String.format(Locale.ROOT, "%s://%s:%s/solr", isHttps ? "https" : "http", host, port);
     }
   }
 }

solr/core/src/java/org/apache/solr/cli/StatusTool.java

@@ -62,7 +62,7 @@ public class StatusTool extends ToolBase {
           .hasArg()
           .argName("PORT")
           .type(Integer.class)
-          .desc("Port on localhost to check status for")
+          .desc("Port of a local Solr process to check status for")
           .get();
 
   public static final Option SHORT_OPTION =
@@ -113,7 +113,6 @@ public void runImpl(CommandLine cli) throws Exception {
 
       // URL provided, do not consult local processes, as the URL may be remote
       if (maxWaitSecs > 0) {
-        // Used by Windows start script when starting Solr
         try {
           waitForSolrUpAndPrintStatus(solrUrl, cli, maxWaitSecs);
           runtime.exit(0);
@@ -128,7 +127,8 @@ public void runImpl(CommandLine cli) throws Exception {
     }
 
     if (port != null) {
-      Optional<SolrProcess> proc = processMgr.processForPort(port);
+      // When asked to wait, this also waits for a newly started process to become visible
+      Optional<SolrProcess> proc = processMgr.waitForProcessOnPort(port, maxWaitSecs);
       if (proc.isEmpty()) {
         CLIO.err("Could not find a running Solr on port " + port);
         runtime.exit(1);
@@ -168,6 +168,7 @@ private void printProcessStatus(SolrProcess process, CommandLine cli) throws Exc
       CLIO.out(pidUrl);
     } else {
       if (maxWaitSecs > 0) {
+        // Used by Windows start script, which passes the port of the newly started instance
         waitForSolrUpAndPrintStatus(pidUrl, cli, maxWaitSecs);
       } else {
         CLIO.out(

solr/core/src/test/org/apache/solr/cli/SolrProcessManagerTest.java

@@ -26,9 +26,12 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.stream.Collectors;
 import org.apache.commons.math3.util.Pair;
 import org.apache.solr.SolrTestCase;
@@ -44,24 +47,34 @@ public class SolrProcessManagerTest extends SolrTestCase {
   private static SolrProcessManager solrProcessManager;
   private static Pair<Integer, Process> processHttp;
   private static Pair<Integer, Process> processHttps;
+  private static Pair<Integer, Process> processBoundHttp;
+  private static Pair<Integer, Process> processAdvertisedHttps;
 
   @BeforeClass
   public static void beforeClass() throws Exception {
     boolean isWindows = random().nextBoolean();
     String PID_SUFFIX = isWindows ? ".port" : ".pid";
     log.info("Simulating pid file on {}", isWindows ? "Windows" : "Linux");
-    processHttp = createProcess(findAvailablePort(), false);
-    processHttps = createProcess(findAvailablePort(), true);
-    long processHttpValue = isWindows ? processHttp.getKey() : processHttp.getValue().pid();
-    long processHttpsValue = isWindows ? processHttps.getKey() : processHttps.getValue().pid();
+    processHttp = createProcess(findAvailablePort(), false, null, null);
+    processHttps = createProcess(findAvailablePort(), true, "127.0.0.1", null);
+    // The mock process does not actually bind to these hosts, they are only command-line markers
+    processBoundHttp = createProcess(findAvailablePort(), false, "10.99.99.99", null);
+    processAdvertisedHttps =
+        createProcess(findAvailablePort(), true, "10.99.99.99", "myhost.example.com");
+    for (Pair<Integer, Process> p :
+        List.of(processHttp, processHttps, processBoundHttp, processAdvertisedHttps)) {
+      awaitReady(p.getValue());
+    }
     SolrProcessManager.enableTestingMode = true;
     System.setProperty("solr.port.listen", Integer.toString(processHttp.getKey()));
     Path pidDir = createTempDir("solr-pid-dir");
     System.setProperty("solr.pid.dir", pidDir.toString());
-    Files.writeString(
-        pidDir.resolve("solr-" + processHttpValue + PID_SUFFIX), Long.toString(processHttpValue));
-    Files.writeString(
-        pidDir.resolve("solr-" + processHttpsValue + PID_SUFFIX), Long.toString(processHttpsValue));
+    for (Pair<Integer, Process> p :
+        List.of(processHttp, processHttps, processBoundHttp, processAdvertisedHttps)) {
+      long pidFileValue = isWindows ? p.getKey() : p.getValue().pid();
+      Files.writeString(
+          pidDir.resolve("solr-" + pidFileValue + PID_SUFFIX), Long.toString(pidFileValue));
+    }
     Files.writeString(pidDir.resolve("solr-99999" + PID_SUFFIX), "99999"); // Invalid
     solrProcessManager = new SolrProcessManager();
   }
@@ -70,6 +83,8 @@ public static void beforeClass() throws Exception {
   public static void afterClass() throws Exception {
     processHttp.getValue().destroyForcibly();
     processHttps.getValue().destroyForcibly();
+    processBoundHttp.getValue().destroyForcibly();
+    processAdvertisedHttps.getValue().destroyForcibly();
     SolrProcessManager.enableTestingMode = false;
   }
 
@@ -80,7 +95,8 @@ private static int findAvailablePort() throws IOException {
   }
 
   @SuppressWarnings("SystemGetProperty")
-  private static Pair<Integer, Process> createProcess(int port, boolean https) throws IOException {
+  private static Pair<Integer, Process> createProcess(
+      int port, boolean https, String bindHost, String advertiseHost) throws IOException {
     // Get the path to the java executable from the current JVM
 
     String pathSeparator = System.getProperty("path.separator");
@@ -89,36 +105,78 @@ private static Pair<Integer, Process> createProcess(int port, boolean https) thr
             .filter(p -> p.contains("solr") && p.contains("core") && p.contains("build"))
             .collect(Collectors.joining(pathSeparator));
 
-    ProcessBuilder processBuilder =
-        new ProcessBuilder(
-            System.getProperty("java.home") + "/bin/java",
-            "-Dsolr.port.listen=" + port,
-            "-DisHttps=" + https,
-            "-DmockSolr=true",
-            "-cp",
-            classPath,
-            "org.apache.solr.cli.SolrProcessManagerTest$MockSolrProcess",
-            https ? "--module=https" : "--module=http");
-
-    // Start the process and read first line of output
-    Process process = processBuilder.start();
+    List<String> command = new ArrayList<>();
+    command.add(System.getProperty("java.home") + "/bin/java");
+    command.add("-Dsolr.port.listen=" + port);
+    command.add("-DisHttps=" + https);
+    command.add("-DmockSolr=true");
+    if (bindHost != null) {
+      command.add("-Dsolr.host.bind=" + bindHost);
+    }
+    if (advertiseHost != null) {
+      command.add("-Dsolr.host.advertise=" + advertiseHost);
+    }
+    command.add("-cp");
+    command.add(classPath);
+    command.add("org.apache.solr.cli.SolrProcessManagerTest$MockSolrProcess");
+    command.add(https ? "--module=https" : "--module=http");
+    return new Pair<>(port, new ProcessBuilder(command).start());
+  }
+
+  /** Waits for the mock process to print its ready line, so the processes can start in parallel */
+  private static void awaitReady(Process process) throws IOException {
     try (InputStream is = process.getInputStream();
         InputStreamReader isr = new InputStreamReader(is, StandardCharsets.UTF_8);
         BufferedReader br = new BufferedReader(isr)) {
       System.out.println(br.readLine());
     }
-    return new Pair<>(port, process);
   }
 
   public void testGetLocalUrl() {
     assertFalse(solrProcessManager.getAllRunning().isEmpty());
-    solrProcessManager
-        .getAllRunning()
-        .forEach(
-            p ->
-                assertEquals(
-                    (p.isHttps() ? "https" : "http") + "://localhost:" + p.port() + "/solr",
-                    p.getLocalUrl()));
+    SolrProcess http = solrProcessManager.processForPort(processHttp.getKey()).orElseThrow();
+    assertEquals("http://localhost:" + http.port() + "/solr", http.getLocalUrl());
+    SolrProcess https = solrProcessManager.processForPort(processHttps.getKey()).orElseThrow();
+    assertEquals("https://localhost:" + https.port() + "/solr", https.getLocalUrl());
+    // Non-loopback bind host is used for the local URL
+    SolrProcess bound = solrProcessManager.processForPort(processBoundHttp.getKey()).orElseThrow();
+    assertEquals("http://10.99.99.99:" + bound.port() + "/solr", bound.getLocalUrl());
+    // Advertised host wins over the bind host
+    SolrProcess advertised =
+        solrProcessManager.processForPort(processAdvertisedHttps.getKey()).orElseThrow();
+    assertEquals(
+        "https://myhost.example.com:" + advertised.port() + "/solr", advertised.getLocalUrl());
+  }
+
+  public void testLocalConnectHost() {
+    // No advertise host: bind host decides, loopback and wildcard binds map to localhost
+    assertEquals("localhost", localConnectHost(null, null));
+    assertEquals("localhost", localConnectHost(null, ""));
+    assertEquals("localhost", localConnectHost(null, "0.0.0.0"));
+    assertEquals("localhost", localConnectHost(null, "::"));
+    assertEquals("localhost", localConnectHost(null, "127.0.0.1"));
+    assertEquals("localhost", localConnectHost(null, "::1"));
+    assertEquals("localhost", localConnectHost(null, "localhost"));
+    assertEquals("10.0.0.5", localConnectHost(null, "10.0.0.5"));
+    assertEquals("myhost.example.com", localConnectHost(null, "myhost.example.com"));
+    assertEquals("[fe80::1]", localConnectHost(null, "fe80::1"));
+    // Advertise host is preferred over the bind host when set
+    assertEquals("myhost.example.com", localConnectHost("myhost.example.com", "10.0.0.5"));
+    assertEquals("myhost.example.com", localConnectHost("myhost.example.com", null));
+    assertEquals("localhost", localConnectHost("localhost", "10.0.0.5"));
+    // Blank advertise host falls back to the bind host
+    assertEquals("10.0.0.5", localConnectHost("", "10.0.0.5"));
+    assertEquals("10.0.0.5", localConnectHost(" ", "10.0.0.5"));
+  }
+
+  private static String localConnectHost(String advertiseHost, String bindHost) {
+    return SolrProcessManager.localConnectHost(
+        Optional.ofNullable(advertiseHost), Optional.ofNullable(bindHost));
+  }
+
+  public void testWaitForProcessOnPort() throws Exception {
+    assertTrue(solrProcessManager.waitForProcessOnPort(processHttp.getKey(), 0).isPresent());
+    assertTrue(solrProcessManager.waitForProcessOnPort(0, 0).isEmpty());
   }
 
   public void testIsRunningWithPort() {
@@ -153,12 +211,12 @@ public void testGetProcessForPid() {
 
   public void testScanSolrPidFiles() throws IOException {
     Collection<SolrProcess> processes = solrProcessManager.scanSolrPidFiles();
-    assertEquals(2, processes.size());
+    assertEquals(4, processes.size());
   }
 
   public void testGetAllRunning() {
     Collection<SolrProcess> processes = solrProcessManager.getAllRunning();
-    assertEquals(2, processes.size());
+    assertEquals(4, processes.size());
   }
 
   public void testSolrProcessMethods() {