Cleanups for ServerLauncher.scala (#5071)

* Fix `DoubleLock` hierarchical locking discipline. This makes the
locking/unlocking order correct, and seems to make some
non-deterministic `OverlappingFileLockException`s stop happening in my
manual testing

* Re-organize `ServerLauncher.run`, folding in `acquireLocksAndRun` into
the main `run` body and breaking out parts of it into helper methods. It
could be greatly simplified since we no longer need to loop over the
possible server dirs to claim one

* Remove `setJnaNoSys` handling, which I *think* is not necessary
anymore now that we have cleaner classpath/process isolation between the
Mill client and server

Author: lihaoyi

runner/client/src/mill/client/ServerCouldNotBeStarted.java

@@ -1,5 +1,6 @@
 package mill.client;
 
+import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.PrintStream;
@@ -47,9 +48,9 @@ public static class Result {
 
   final int serverInitWaitMillis = 10000;
 
-  public abstract void initServer(Path serverDir, boolean b, Locks locks) throws Exception;
+  public abstract void initServer(Path serverDir, Locks locks) throws Exception;
 
-  public abstract void preRun(Path serverDir) throws Exception;
+  public abstract void prepareServerDir(Path serverDir) throws Exception;
 
   InputStream stdin;
   PrintStream stdout;
@@ -81,25 +82,34 @@ public ServerLauncher(
     this.forceFailureForTestingMillisDelay = forceFailureForTestingMillisDelay;
   }
 
-  public Result acquireLocksAndRun(Path serverDir) throws Exception {
-    final boolean setJnaNoSys = System.getProperty("jna.nosys") == null;
-    if (setJnaNoSys) System.setProperty("jna.nosys", "true");
+  public Result run(Path serverDir) throws Exception {
 
     Files.createDirectories(serverDir);
 
+    prepareServerDir(serverDir);
+
+    Socket ioSocket = launchConnectToServer(serverDir);
+
+    try {
+      Thread outPumperThread = startStreamPumpers(ioSocket);
+      forceTestFailure(serverDir);
+      outPumperThread.join();
+    } finally {
+      ioSocket.close();
+    }
+
     Result result = new Result();
-    preRun(serverDir);
-    result.exitCode = run(serverDir, setJnaNoSys);
+    result.exitCode = readExitCode(serverDir);
     result.serverDir = serverDir;
     return result;
   }
 
-  int run(Path serverDir, boolean setJnaNoSys) throws Exception {
+  Socket launchConnectToServer(Path serverDir) throws Exception {
 
     try (Locks locks = memoryLock != null ? memoryLock : Locks.files(serverDir.toString());
         mill.client.lock.Locked locked = locks.clientLock.lock()) {
 
-      if (locks.serverLock.probe()) initServer(serverDir, setJnaNoSys, locks);
+      if (locks.serverLock.probe()) initServer(serverDir, locks);
       while (locks.serverLock.probe()) Thread.sleep(1);
     }
     long retryStart = System.currentTimeMillis();
@@ -114,11 +124,20 @@ int run(Path serverDir, boolean setJnaNoSys) throws Exception {
         Thread.sleep(1);
       }
     }
-
     if (ioSocket == null) {
       throw new Exception("Failed to connect to server", socketThrowable);
     }
+    return ioSocket;
+  }
 
+  private void forceTestFailure(Path serverDir) throws Exception {
+    if (forceFailureForTestingMillisDelay > 0) {
+      Thread.sleep(forceFailureForTestingMillisDelay);
+      throw new Exception("Force failure for testing: " + serverDir);
+    }
+  }
+
+  Thread startStreamPumpers(Socket ioSocket) throws Exception {
     InputStream outErr = ioSocket.getInputStream();
     OutputStream in = ioSocket.getOutputStream();
     in.write(Util.hasConsole() ? 1 : 0);
@@ -133,23 +152,16 @@ int run(Path serverDir, boolean setJnaNoSys) throws Exception {
     inThread.setDaemon(true);
     outPumperThread.start();
     inThread.start();
+    return outPumperThread;
+  }
 
-    try {
-      if (forceFailureForTestingMillisDelay > 0) {
-        Thread.sleep(forceFailureForTestingMillisDelay);
-        throw new Exception("Force failure for testing: " + serverDir);
-      }
-      outPumperThread.join();
-
-      Path exitCodeFile = serverDir.resolve(ServerFiles.exitCode);
-      if (Files.exists(exitCodeFile)) {
-        return Integer.parseInt(Files.readAllLines(exitCodeFile).get(0));
-      } else {
-        System.err.println("mill-server/ exitCode file not found");
-        return 1;
-      }
-    } finally {
-      ioSocket.close();
+  int readExitCode(Path serverDir) throws IOException {
+    Path exitCodeFile = serverDir.resolve(ServerFiles.exitCode);
+    if (Files.exists(exitCodeFile)) {
+      return Integer.parseInt(Files.readAllLines(exitCodeFile).get(0));
+    } else {
+      System.err.println("mill-server/ exitCode file not found");
+      return 1;
     }
   }
 }

runner/client/src/mill/client/ServerLauncher.java

@@ -22,8 +22,10 @@ public TryLocked tryLock() throws Exception {
     if (l1.isLocked() && l2.isLocked()) {
       return new DoubleTryLocked(l1, l2);
     } else {
-      l1.release();
+      // Unlock the locks in the opposite order in which we originally took them
       l2.release();
+      l1.release();
+
       return new DoubleTryLocked(null, null);
     }
   }
@@ -40,8 +42,9 @@ public boolean probe() throws Exception {
 
   @Override
   public void close() throws Exception {
-    lock1.close();
+    // Unlock the locks in the opposite order in which we originally took them
     lock2.close();
+    lock1.close();
   }
 
   @Override

runner/client/src/mill/client/lock/DoubleLock.java

@@ -19,8 +19,10 @@ public Locked lock() {
 
   @Override
   public MemoryTryLocked tryLock() {
-    if (innerLock.tryLock()) return new MemoryTryLocked(innerLock);
-    else return new MemoryTryLocked(null);
+    MemoryTryLocked res =
+        innerLock.tryLock() ? new MemoryTryLocked(innerLock) : new MemoryTryLocked(null);
+
+    return res;
   }
 
   @Override

runner/client/src/mill/client/lock/MemoryLock.java

@@ -50,35 +50,23 @@ public static void main(String[] args) throws Exception {
                 optsArgs.toArray(new String[0]),
                 null,
                 -1) {
-              public void initServer(Path serverDir, boolean setJnaNoSys, Locks locks)
-                  throws Exception {
-                MillProcessLauncher.launchMillServer(serverDir, setJnaNoSys);
+              public void initServer(Path serverDir, Locks locks) throws Exception {
+                MillProcessLauncher.launchMillServer(serverDir);
               }
 
-              public void preRun(Path serverDir) throws Exception {
+              public void prepareServerDir(Path serverDir) throws Exception {
                 MillProcessLauncher.prepareMillRunFolder(serverDir);
               }
             };
 
         final String versionAndJvmHomeEncoding =
             Util.md5hex(mill.client.BuildInfo.millVersion + MillProcessLauncher.javaHome());
         Path serverDir0 = Paths.get(OutFiles.out, OutFiles.millServer, versionAndJvmHomeEncoding);
-        int exitCode = launcher.acquireLocksAndRun(serverDir0).exitCode;
+        int exitCode = launcher.run(serverDir0).exitCode;
         if (exitCode == ClientUtil.ExitServerCodeWhenVersionMismatch()) {
-          exitCode = launcher.acquireLocksAndRun(serverDir0).exitCode;
+          exitCode = launcher.run(serverDir0).exitCode;
         }
         System.exit(exitCode);
-      } catch (ServerCouldNotBeStarted e) {
-        // TODO: try to run in-process
-        System.err.println("Could not start a Mill server process.\n"
-            + "This could be caused by too many already running Mill instances "
-            + "or by an unsupported platform.\n"
-            + e.getMessage() + "\n");
-
-        System.err.println(
-            "Loading Mill in-process isn't possible.\n" + "Please check your Mill installation!");
-        throw e;
-
       } catch (Exception e) {
         System.err.println("Mill client failed with unknown exception");
         e.printStackTrace();

runner/launcher/src/mill/launcher/MillLauncherMain.java

@@ -23,12 +23,11 @@
 public class MillProcessLauncher {
 
   static int launchMillNoServer(String[] args) throws Exception {
-    final boolean setJnaNoSys = System.getProperty("jna.nosys") == null;
     final String sig = String.format("%08x", UUID.randomUUID().hashCode());
     final Path processDir = Paths.get(".").resolve(out).resolve(millNoServer).resolve(sig);
 
     final List<String> l = new ArrayList<>();
-    l.addAll(millLaunchJvmCommand(setJnaNoSys));
+    l.addAll(millLaunchJvmCommand());
     l.add("mill.daemon.MillMain");
     l.add(processDir.toAbsolutePath().toString());
     l.addAll(millOpts());
@@ -57,9 +56,9 @@ static int launchMillNoServer(String[] args) throws Exception {
     }
   }
 
-  static void launchMillServer(Path serverDir, boolean setJnaNoSys) throws Exception {
+  static void launchMillServer(Path serverDir) throws Exception {
     List<String> l = new ArrayList<>();
-    l.addAll(millLaunchJvmCommand(setJnaNoSys));
+    l.addAll(millLaunchJvmCommand());
     l.add("mill.daemon.MillServerMain");
     l.add(serverDir.toFile().getCanonicalPath());
 
@@ -202,17 +201,12 @@ static String javaExe() throws Exception {
     }
   }
 
-  static List<String> millLaunchJvmCommand(boolean setJnaNoSys) throws Exception {
+  static List<String> millLaunchJvmCommand() throws Exception {
     final List<String> vmOptions = new ArrayList<>();
 
     // Java executable
     vmOptions.add(javaExe());
 
-    // jna
-    if (setJnaNoSys) {
-      vmOptions.add("-Djna.nosys=true");
-    }
-
     // sys props
     final Properties sysProps = System.getProperties();
     for (final String k : sysProps.stringPropertyNames()) {

runner/launcher/src/mill/launcher/MillProcessLauncher.java

@@ -95,8 +95,8 @@ object ClientServerTests extends TestSuite {
         memoryLock,
         forceFailureForTestingMillisDelay
       ) {
-        def preRun(serverDir: Path) = { /*do nothing*/ }
-        def initServer(serverDir: Path, b: Boolean, locks: Locks) = {
+        def prepareServerDir(serverDir: Path) = { /*do nothing*/ }
+        def initServer(serverDir: Path, locks: Locks) = {
           val processId = "server-" + nextServerId
           nextServerId += 1
           new Thread(new EchoServer(
@@ -106,7 +106,7 @@ object ClientServerTests extends TestSuite {
             testLogEvenWhenServerIdWrong
           )).start()
         }
-      }.acquireLocksAndRun((outDir / "server-0").relativeTo(os.pwd).toNIO)
+      }.run((outDir / "server-0").relativeTo(os.pwd).toNIO)
 
       ClientResult(
         result.exitCode,