8042088: Sjavac spawns external processes in a unnecessarily complex and platform dependent way
authoralundblad
Mon, 03 Nov 2014 10:20:34 +0100
changeset 27383 716ed9a6d607
parent 27382 60a3693d0d87
child 27384 51aa4299ae7b
8042088: Sjavac spawns external processes in a unnecessarily complex and platform dependent way Summary: Refactoring of the sjavac background fork code. Reviewed-by: jfranck, ohrstrom
langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/CompileJavaPackages.java
langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/client/SjavacClient.java
langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/options/OptionHelper.java
langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/PortFile.java
langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/PortFileMonitor.java
langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/ServerMain.java
langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/SjavacServer.java
--- a/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/CompileJavaPackages.java	Fri Oct 31 17:23:21 2014 -0600
+++ b/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/CompileJavaPackages.java	Mon Nov 03 10:20:34 2014 +0100
@@ -91,7 +91,7 @@
 
         // Get maximum heap size from the server!
         SysInfo sysinfo = sjavac.getSysInfo();
-        if (sysinfo.numCores == -1) {
+        if (sysinfo == null) {
             Log.error("Could not query server for sysinfo!");
             return false;
         }
--- a/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/client/SjavacClient.java	Fri Oct 31 17:23:21 2014 -0600
+++ b/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/client/SjavacClient.java	Mon Nov 03 10:20:34 2014 +0100
@@ -25,31 +25,33 @@
 
 package com.sun.tools.sjavac.client;
 
-import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileNotFoundException;
-import java.io.FileReader;
 import java.io.IOException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
+import java.io.PrintStream;
 import java.io.PrintWriter;
 import java.io.StringWriter;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.Socket;
 import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
+import java.util.Scanner;
 import java.util.Set;
 
 import com.sun.tools.sjavac.Log;
-import com.sun.tools.sjavac.ProblemException;
 import com.sun.tools.sjavac.Util;
+import com.sun.tools.sjavac.options.OptionHelper;
+import com.sun.tools.sjavac.options.Options;
 import com.sun.tools.sjavac.server.CompilationResult;
 import com.sun.tools.sjavac.server.PortFile;
 import com.sun.tools.sjavac.server.Sjavac;
 import com.sun.tools.sjavac.server.SjavacServer;
 import com.sun.tools.sjavac.server.SysInfo;
-import com.sun.tools.sjavac.options.Options;
 
 /**
  * Sjavac implementation that delegates requests to a SjavacServer.
@@ -64,10 +66,9 @@
     // The id can perhaps be used in the future by the javac server to reuse the
     // JavaCompiler instance for several compiles using the same id.
     private final String id;
-    private final String portfileName;
+    private final PortFile portFile;
     private final String logfile;
     private final String stdouterrfile;
-    private final boolean background;
 
     // Default keepalive for server is 120 seconds.
     // I.e. it will accept 120 seconds of inactivity before quitting.
@@ -88,16 +89,27 @@
     // Store the server conf settings here.
     private final String settings;
 
-    public SjavacClient(Options options) {
+    // This constructor should not throw FileNotFoundException (to be resolved
+    // in JDK-8060030)
+    public SjavacClient(Options options) throws FileNotFoundException {
         String tmpServerConf = options.getServerConf();
         String serverConf = (tmpServerConf!=null)? tmpServerConf : "";
         String tmpId = Util.extractStringOption("id", serverConf);
         id = (tmpId!=null) ? tmpId : "id"+(((new java.util.Random()).nextLong())&Long.MAX_VALUE);
-        String p = Util.extractStringOption("portfile", serverConf);
-        portfileName = (p!=null) ? p : options.getStateDir().toFile().getAbsolutePath()+File.separatorChar+"javac_server";
+        String defaultPortfile = options.getStateDir()
+                                        .resolve("javac_server")
+                                        .toAbsolutePath()
+                                        .toString();
+        String portfileName = Util.extractStringOption("portfile", serverConf, defaultPortfile);
+        try {
+            portFile = SjavacServer.getPortFile(portfileName);
+        } catch (FileNotFoundException e) {
+            // Reached for instance if directory of port file does not exist
+            Log.error("Port file inaccessable: " + e);
+            throw e;
+        }
         logfile = Util.extractStringOption("logfile", serverConf, portfileName + ".javaclog");
         stdouterrfile = Util.extractStringOption("stdouterrfile", serverConf, portfileName + ".stdouterr");
-        background = Util.extractBooleanOption("background", serverConf, true);
         sjavacForkCmd = Util.extractStringOption("sjavac", serverConf, "sjavac");
         int poolsize = Util.extractIntOption("poolsize", serverConf);
         keepalive = Util.extractIntOption("keepalive", serverConf, 120);
@@ -138,8 +150,11 @@
             return (SysInfo) ois.readObject();
         } catch (IOException | ClassNotFoundException ex) {
             Log.error("[CLIENT] Exception caught: " + ex);
-            StringWriter sw = new StringWriter();
-            ex.printStackTrace(new PrintWriter(sw));
+            Log.debug(Util.getStackTrace(ex));
+        } catch (InterruptedException ie) {
+            Thread.currentThread().interrupt(); // Restore interrupt
+            Log.error("[CLIENT] getSysInfo interrupted.");
+            Log.debug(Util.getStackTrace(ie));
         }
         return null;
     }
@@ -170,106 +185,127 @@
             oos.flush();
             result = (CompilationResult) ois.readObject();
         } catch (IOException | ClassNotFoundException ex) {
-            Log.error("Exception caught: " + ex);
+            Log.error("[CLIENT] Exception caught: " + ex);
             result = new CompilationResult(CompilationResult.ERROR_FATAL);
             result.stderr = Util.getStackTrace(ex);
+        } catch (InterruptedException ie) {
+            Thread.currentThread().interrupt(); // Restore interrupt
+            Log.error("[CLIENT] compile interrupted.");
+            result = new CompilationResult(CompilationResult.ERROR_FATAL);
+            result.stderr = Util.getStackTrace(ie);
         }
         return result;
     }
 
-    private Socket tryConnect() throws IOException {
-
-        PortFile portFile;
-        try {
-            // This should be taken care of at a higher level (JDK-8048451)
-            portFile = SjavacServer.getPortFile(portfileName);
-        } catch (FileNotFoundException e) {
-            // Reached for instance if directory of port file does not exist
-            Log.error("Port file inaccessable: " + e);
-            throw new RuntimeException(e);
-        }
-        for (int i = 0; i < MAX_CONNECT_ATTEMPTS; i++) {
-            Log.info(String.format("Trying to connect (attempt %d of %d)",
-                                   i+1, MAX_CONNECT_ATTEMPTS));
+    /*
+     * Makes MAX_CONNECT_ATTEMPTS attepmts to connect to server.
+     */
+    private Socket tryConnect() throws IOException, InterruptedException {
+        makeSureServerIsRunning(portFile);
+        int attempt = 0;
+        while (true) {
+            Log.info("Trying to connect. Attempt " + (++attempt) + " of " + MAX_CONNECT_ATTEMPTS);
             try {
-                if (!makeSureServerIsRunning(portFile))
-                    continue;
-                Socket socket = new Socket();
-                InetAddress localhost = InetAddress.getByName(null);
-                socket.connect(new InetSocketAddress(localhost, portFile.getPort()),
-                               CONNECTION_TIMEOUT);
-                return socket;
-            } catch (ProblemException | IOException ex) {
-                Log.error("Caught exception during tryConnect: " + ex);
+                return makeConnectionAttempt();
+            } catch (IOException ex) {
+                Log.error("Connection attempt failed: " + ex.getMessage());
+                if (attempt >= MAX_CONNECT_ATTEMPTS) {
+                    Log.error("Giving up");
+                    throw new IOException("Could not connect to server", ex);
+                }
             }
-
-            try {
-                Thread.sleep(WAIT_BETWEEN_CONNECT_ATTEMPTS);
-            } catch (InterruptedException e) {
-                Thread.currentThread().interrupt();
-            }
+            Thread.sleep(WAIT_BETWEEN_CONNECT_ATTEMPTS);
         }
-        throw new IOException("Could not connect to server");
     }
 
-    private boolean makeSureServerIsRunning(PortFile portFile)
-            throws IOException, ProblemException, FileNotFoundException {
+    private Socket makeConnectionAttempt() throws IOException {
+        Socket socket = new Socket();
+        InetAddress localhost = InetAddress.getByName(null);
+        InetSocketAddress address = new InetSocketAddress(localhost, portFile.getPort());
+        socket.connect(address, CONNECTION_TIMEOUT);
+        Log.info("Connected");
+        return socket;
+    }
 
-        synchronized (portFile) {
-            portFile.lock();
-            portFile.getValues();
-            portFile.unlock();
+    /*
+     * Will return immediately if a server already seems to be running,
+     * otherwise fork a new server and block until it seems to be running.
+     */
+    private void makeSureServerIsRunning(PortFile portFile)
+            throws IOException, InterruptedException {
+
+        portFile.lock();
+        portFile.getValues();
+        portFile.unlock();
+
+        if (portFile.containsPortInfo()) {
+            // Server seems to already be running
+            return;
         }
 
-        if (!portFile.containsPortInfo()) {
-            String forkCmd = SjavacServer.fork(sjavacForkCmd,
-                                               portFile.getFilename(),
-                                               logfile,
-                                               poolsize,
-                                               keepalive,
-                                               System.err,
-                                               stdouterrfile,
-                                               background);
-            if (!portFile.waitForValidValues()) {
-                // This can be simplified once JDK-8048457 has been addressed
-                // since we won't have an SjavacClient if background = false
-                if (background) {
-                    // There seems be some problem with spawning the external
-                    // process (for instance no fork command provided and no
-                    // sjavac on path)
-                    StringWriter sw = new StringWriter();
-                    SjavacClient.printFailedAttempt(forkCmd,
-                                                    stdouterrfile,
-                                                    new PrintWriter(sw));
-                    Log.error(sw.toString());
-                }
-            }
-        }
-        return portFile.containsPortInfo();
-    }
-
-
-    public static void printFailedAttempt(String cmd, String f, PrintWriter err) {
-        err.println("---- Failed to start javac server with this command -----");
-        err.println(cmd);
-        try {
-            BufferedReader in = new BufferedReader(new FileReader(f));
-            err.println("---- stdout/stderr output from attempt to start javac server -----");
-            for (;;) {
-                String l = in.readLine();
-                if (l == null) {
-                    break;
-                }
-                err.println(l);
-            }
-            err.println("------------------------------------------------------------------");
-        } catch (Exception e) {
-            err.println("The stdout/stderr output in file " + f + " does not exist and the server did not start.");
-        }
+        // Fork a new server and wait for it to start
+        SjavacClient.fork(sjavacForkCmd,
+                          portFile,
+                          logfile,
+                          poolsize,
+                          keepalive,
+                          System.err,
+                          stdouterrfile);
     }
 
     @Override
     public void shutdown() {
         // Nothing to clean up
     }
+
+    /*
+     * Fork a server process process and wait for server to come around
+     */
+    public static void fork(String sjavacCmd,
+                            PortFile portFile,
+                            String logfile,
+                            int poolsize,
+                            int keepalive,
+                            final PrintStream err,
+                            String stdouterrfile)
+                                    throws IOException, InterruptedException {
+        List<String> cmd = new ArrayList<>();
+        cmd.addAll(Arrays.asList(OptionHelper.unescapeCmdArg(sjavacCmd).split(" ")));
+        cmd.add("--startserver:"
+              + "portfile=" + portFile.getFilename()
+              + ",logfile=" + logfile
+              + ",stdouterrfile=" + stdouterrfile
+              + ",poolsize=" + poolsize
+              + ",keepalive="+ keepalive);
+
+        Process p = null;
+        Log.info("Starting server. Command: " + String.join(" ", cmd));
+        try {
+            // If the cmd for some reason can't be executed (file not found, or
+            // is not executable) this will throw an IOException with a decent
+            // error message.
+            p = new ProcessBuilder(cmd)
+                        .redirectErrorStream(true)
+                        .redirectOutput(new File(stdouterrfile))
+                        .start();
+
+            // Throws an IOException if no valid values materialize
+            portFile.waitForValidValues();
+
+        } catch (IOException ex) {
+            // Log and rethrow exception
+            Log.error("Faild to launch server.");
+            Log.error("    Message: " + ex.getMessage());
+            String rc = p == null || p.isAlive() ? "n/a" : "" + p.exitValue();
+            Log.error("    Server process exit code: " + rc);
+            Log.error("Server log:");
+            Log.error("------- Server log start -------");
+            try (Scanner s = new Scanner(new File(stdouterrfile))) {
+                while (s.hasNextLine())
+                    Log.error(s.nextLine());
+            }
+            Log.error("------- Server log end ---------");
+            throw ex;
+        }
+    }
 }
--- a/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/options/OptionHelper.java	Fri Oct 31 17:23:21 2014 -0600
+++ b/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/options/OptionHelper.java	Mon Nov 03 10:20:34 2014 +0100
@@ -25,7 +25,6 @@
 
 package com.sun.tools.sjavac.options;
 
-import java.io.IOException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Arrays;
@@ -159,4 +158,9 @@
             }
         }
     }
+
+    public static String unescapeCmdArg(String arg) {
+        return arg.replaceAll("%20", " ")
+                  .replaceAll("%2C", ",");
+    }
 }
--- a/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/PortFile.java	Fri Oct 31 17:23:21 2014 -0600
+++ b/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/PortFile.java	Mon Nov 03 10:20:34 2014 +0100
@@ -33,6 +33,8 @@
 import java.nio.channels.FileChannel;
 import java.nio.channels.FileLock;
 import java.nio.channels.FileLockInterruptionException;
+import java.util.concurrent.Semaphore;
+
 import com.sun.tools.javac.util.Assert;
 import com.sun.tools.sjavac.Log;
 
@@ -61,7 +63,12 @@
     private File stopFile;
     private RandomAccessFile rwfile;
     private FileChannel channel;
+
+    // FileLock used to solve inter JVM synchronization, lockSem used to avoid
+    // JVM internal OverlappingFileLockExceptions.
+    // Class invariant: lock.isValid() <-> lockSem.availablePermits() == 0
     private FileLock lock;
+    private Semaphore lockSem = new Semaphore(1);
 
     private boolean containsPortInfo;
     private int serverPort;
@@ -88,7 +95,8 @@
     /**
      * Lock the port file.
      */
-    public void lock() throws IOException {
+    public void lock() throws IOException, InterruptedException {
+        lockSem.acquire();
         lock = channel.lock();
     }
 
@@ -195,34 +203,37 @@
         Assert.check(lock != null);
         lock.release();
         lock = null;
+        lockSem.release();
     }
 
     /**
      * Wait for the port file to contain values that look valid.
-     * Return true, if a-ok, false if the valid values did not materialize within 5 seconds.
      */
-    public synchronized boolean waitForValidValues() throws IOException, FileNotFoundException {
-        for (int tries = 0; tries < 50; tries++) {
+    public void waitForValidValues() throws IOException, InterruptedException {
+        final int MAX_ATTEMPTS = 10;
+        final int MS_BETWEEN_ATTEMPTS = 500;
+        long startTime = System.currentTimeMillis();
+        for (int attempt = 0; ; attempt++) {
+            Log.debug("Looking for valid port file values...");
             lock();
             getValues();
             unlock();
             if (containsPortInfo) {
-                Log.debug("Found valid values in port file after waiting "+(tries*100)+"ms");
-                return true;
+                Log.debug("Valid port file values found after " + (System.currentTimeMillis() - startTime) + " ms");
+                return;
             }
-            try {
-                Thread.sleep(100);
-            } catch (InterruptedException e)
-            {}
+            if (attempt >= MAX_ATTEMPTS) {
+                throw new IOException("No port file values materialized. Giving up after " +
+                                      (System.currentTimeMillis() - startTime) + " ms");
+            }
+            Thread.sleep(MS_BETWEEN_ATTEMPTS);
         }
-        Log.debug("Gave up waiting for valid values in port file");
-        return false;
     }
 
     /**
      * Check if the portfile still contains my values, assuming that I am the server.
      */
-    public synchronized boolean stillMyValues() throws IOException, FileNotFoundException {
+    public boolean stillMyValues() throws IOException, FileNotFoundException, InterruptedException {
         for (;;) {
             try {
                 lock();
--- a/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/PortFileMonitor.java	Fri Oct 31 17:23:21 2014 -0600
+++ b/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/PortFileMonitor.java	Mon Nov 03 10:20:34 2014 +0100
@@ -75,6 +75,10 @@
                 } catch (IOException e) {
                     e.printStackTrace(server.theLog);
                     server.flushLog();
+                } catch (InterruptedException e) {
+                    Thread.currentThread().interrupt();
+                    e.printStackTrace(server.theLog);
+                    server.flushLog();
                 }
             }
         };
--- a/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/ServerMain.java	Fri Oct 31 17:23:21 2014 -0600
+++ b/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/ServerMain.java	Mon Nov 03 10:20:34 2014 +0100
@@ -46,8 +46,8 @@
         try {
             SjavacServer server = new SjavacServer(args[0], System.err);
             exitCode = server.startServer();
-        } catch (IOException ioex) {
-            ioex.printStackTrace();
+        } catch (IOException | InterruptedException ex) {
+            ex.printStackTrace();
             exitCode = -1;
         }
 
--- a/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/SjavacServer.java	Fri Oct 31 17:23:21 2014 -0600
+++ b/langtools/src/jdk.compiler/share/classes/com/sun/tools/sjavac/server/SjavacServer.java	Mon Nov 03 10:20:34 2014 +0100
@@ -24,7 +24,6 @@
  */
 package com.sun.tools.sjavac.server;
 
-import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.PrintStream;
@@ -34,16 +33,14 @@
 import java.net.ServerSocket;
 import java.net.Socket;
 import java.net.SocketException;
-import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Random;
 import java.util.concurrent.atomic.AtomicBoolean;
 
-import com.sun.tools.sjavac.ProblemException;
 import com.sun.tools.sjavac.Util;
+import com.sun.tools.sjavac.comp.PooledSjavac;
 import com.sun.tools.sjavac.comp.SjavacImpl;
-import com.sun.tools.sjavac.comp.PooledSjavac;
 
 /**
  * The JavacServer class contains methods both to setup a server that responds to requests and methods to connect to this server.
@@ -95,13 +92,26 @@
     private static Map<String, Long> maxServerMemory;
 
     public SjavacServer(String settings, PrintStream err) throws FileNotFoundException {
-        // Extract options. TODO: Change to proper constructor args
-        portfilename = Util.extractStringOption("portfile", settings);
-        logfile = Util.extractStringOption("logfile", settings);
-        stdouterrfile = Util.extractStringOption("stdouterrfile", settings);
-        keepalive = Util.extractIntOption("keepalive", settings, 120);
-        poolsize = Util.extractIntOption("poolsize", settings,
-                                         Runtime.getRuntime().availableProcessors());
+        this(Util.extractStringOption("portfile", settings),
+             Util.extractStringOption("logfile", settings),
+             Util.extractStringOption("stdouterrfile", settings),
+             Util.extractIntOption("poolsize", settings, Runtime.getRuntime().availableProcessors()),
+             Util.extractIntOption("keepalive", settings, 120),
+             err);
+    }
+
+    public SjavacServer(String portfilename,
+                        String logfile,
+                        String stdouterrfile,
+                        int poolsize,
+                        int keepalive,
+                        PrintStream err)
+                                throws FileNotFoundException {
+        this.portfilename = portfilename;
+        this.logfile = logfile;
+        this.stdouterrfile = stdouterrfile;
+        this.poolsize = poolsize;
+        this.keepalive = keepalive;
         this.err = err;
 
         myCookie = new Random().nextLong();
@@ -180,7 +190,7 @@
      * Start a server using a settings string. Typically: "--startserver:portfile=/tmp/myserver,poolsize=3" and the string "portfile=/tmp/myserver,poolsize=3"
      * is sent as the settings parameter. Returns 0 on success, -1 on failure.
      */
-    public int startServer() throws IOException {
+    public int startServer() throws IOException, InterruptedException {
         long serverStart = System.currentTimeMillis();
 
         // The port file is locked and the server port and cookie is written into it.
@@ -250,64 +260,6 @@
         return 0;
     }
 
-    /**
-     * Fork a background process. Returns the command line used that can be printed if something failed.
-     */
-    public static String fork(String sjavac, String portfile, String logfile, int poolsize, int keepalive,
-            final PrintStream err, String stdouterrfile, boolean background)
-            throws IOException, ProblemException {
-        if (stdouterrfile != null && stdouterrfile.trim().equals("")) {
-            stdouterrfile = null;
-        }
-        final String startserver = "--startserver:portfile=" + portfile + ",logfile=" + logfile + ",stdouterrfile=" + stdouterrfile + ",poolsize=" + poolsize + ",keepalive="+ keepalive;
-
-        if (background) {
-            sjavac += "%20" + startserver;
-            sjavac = sjavac.replaceAll("%20", " ");
-            sjavac = sjavac.replaceAll("%2C", ",");
-            // If the java/sh/cmd launcher fails the failure will be captured by stdouterr because of the redirection here.
-            String[] cmd = {"/bin/sh", "-c", sjavac + " >> " + stdouterrfile + " 2>&1"};
-            if (!(new File("/bin/sh")).canExecute()) {
-                ArrayList<String> wincmd = new ArrayList<>();
-                wincmd.add("cmd");
-                wincmd.add("/c");
-                wincmd.add("start");
-                wincmd.add("cmd");
-                wincmd.add("/c");
-                wincmd.add(sjavac + " >> " + stdouterrfile + " 2>&1");
-                cmd = wincmd.toArray(new String[wincmd.size()]);
-            }
-            Process pp = null;
-            try {
-                pp = Runtime.getRuntime().exec(cmd);
-            } catch (Exception e) {
-                e.printStackTrace(err);
-                e.printStackTrace(new PrintWriter(stdouterrfile));
-            }
-            StringBuilder rs = new StringBuilder();
-            for (String s : cmd) {
-                rs.append(s + " ");
-            }
-            return rs.toString();
-        }
-
-        // Do not spawn a background server, instead run it within the same JVM.
-        Thread t = new Thread() {
-            @Override
-            public void run() {
-                try {
-                    SjavacServer server = new SjavacServer(startserver, err);
-                    server.startServer();
-                } catch (Throwable t) {
-                    t.printStackTrace(err);
-                }
-            }
-        };
-        t.setDaemon(true);
-        t.start();
-        return "";
-    }
-
     @Override
     public void shutdown(String quitMsg) {
         if (!keepAcceptingRequests.compareAndSet(true, false)) {