JNLP slave agents is made more robust in the face of configuration errors.

    (<a href="http://d.hatena.ne.jp/w650/20090107/1231323990">report</a>)


git-svn-id: https://hudson.dev.java.net/svn/hudson/trunk/hudson/main@14337 71c3de6d-444a-0410-be80-ed276b4c234a

core/src/main/java/hudson/lifecycle/WindowsSlaveInstaller.java

@@ -93,12 +93,12 @@ public class WindowsSlaveInstaller implements Callable<Void,RuntimeException>, A
             // write out the descriptor
             String xml = IOUtils.toString(getClass().getResourceAsStream("/windows-service/hudson-slave.xml"), "UTF-8");
             xml = xml.replace("@JAVA@",System.getProperty("java.home")+"\\bin\\java.exe");
-            URL jnlp = new URL(new URL(engine.hudsonUrl),"../computer/"+engine.slaveName+"/slave-agent.jnlp");
+            URL jnlp = new URL(engine.getHudsonUrl(),"../computer/"+engine.slaveName+"/slave-agent.jnlp");
             xml = xml.replace("@URL@",jnlp.toExternalForm());
             FileUtils.writeStringToFile(new File(dir, "hudson-slave.xml"),xml,"UTF-8");
 
             // copy slave.jar
-            URL slaveJar = new URL(new URL(engine.hudsonUrl),"../jnlpJars/remoting.jar");
+            URL slaveJar = new URL(engine.getHudsonUrl(),"../jnlpJars/remoting.jar");
             File dstSlaveJar = new File(dir,"slave.jar").getCanonicalFile();
             if(!dstSlaveJar.exists()) // perhaps slave.jar is already there?
                 FileUtils.copyURLToFile(slaveJar,dstSlaveJar);

core/src/main/resources/hudson/slaves/SlaveComputer/slave-agent.jnlp.jelly

@@ -36,14 +36,28 @@
       </resources>
 
       <application-desc main-class="hudson.remoting.jnlp.Main">
-        <argument>${h.getServerName()}</argument>
-        <argument>${rootURL}tcpSlaveAgentListener/</argument>
         <argument>${app.secretKey}</argument>
         <argument>${it.node.nodeName}</argument>
         <j:if test="${it.launcher.tunnel!=null}">
           <argument>-tunnel</argument>
           <argument>${it.launcher.tunnel}</argument>
         </j:if>
+
+        <argument>-url</argument>
+        <argument>${rootURL}</argument>
+
+        <!--
+          rootURL is based on the URL in the system config, but there has been
+          numerous reports about people moving Hudson to another place but
+          forgetting to update it. To improve the user experience in this regard,
+          let's also pass the URL that the browser sent us as well, so that the
+          JNLP Main class can try both.
+
+          Note that rootURL is still necessary in various situations, such
+          as reverse HTTP proxy situation, which makes rootUrlFromRequest incorrect. 
+        -->
+        <argument>-url</argument>
+        <argument>${app.rootUrlFromRequest}</argument>
       </application-desc>
     </jnlp>
   </l:isAdminOrTest>

remoting/pom.xml

@@ -93,7 +93,7 @@
     <dependency>
       <groupId>args4j</groupId>
       <artifactId>args4j</artifactId>
-      <version>2.0.9</version>
+      <version>2.0.10</version>
       <scope>provided</scope>
     </dependency>
   </dependencies>

remoting/src/main/java/hudson/remoting/Engine.java

@@ -10,6 +10,8 @@ import java.net.URL;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ThreadFactory;
+import java.util.List;
+import java.util.Collections;
 
 /**
  * Slave agent engine that proactively connects to Hudson master.
@@ -33,14 +35,24 @@ public class Engine extends Thread {
     });
 
     public final EngineListener listener;
+
     /**
-     * Host name of Hudson master to connect to, when opening a TCP connection.
+     * To make Hudson more graceful against user error,
+     * JNLP agent can try to connect to multiple possible Hudson URLs.
+     * This field specifies those candidate URLs, such as
+     * "http://foo.bar/hudson/".
      */
-    public final String host;
+    private List<URL> candidateUrls;
+
     /**
      * URL that points to Hudson's tcp slage agent listener, like <tt>http://myhost/hudson/tcpSlaveAgentListener/</tt>
+     *
+     * <p>
+     * This value is determined from {@link #candidateUrls} after a successful connection.
+     * Note that this URL also has "tcpSlaveAgentListener" in it.
      */
-    public final String hudsonUrl;
+    private URL hudsonUrl;
+
     private final String secretKey;
     public final String slaveName;
 
@@ -49,12 +61,17 @@ public class Engine extends Thread {
      */
     private String tunnel;
 
-    public Engine(EngineListener listener, String host, String hudsonUrl, String secretKey, String slaveName) {
+    public Engine(EngineListener listener, List<URL> hudsonUrls, String secretKey, String slaveName) {
         this.listener = listener;
-        this.host = host;
-        this.hudsonUrl = hudsonUrl;
+        this.candidateUrls = hudsonUrls;
         this.secretKey = secretKey;
         this.slaveName = slaveName;
+        if(candidateUrls.isEmpty())
+            throw new IllegalArgumentException("No URLs given");
+    }
+
+    public URL getHudsonUrl() {
+        return hudsonUrl;
     }
 
     public void setTunnel(String tunnel) {
@@ -65,16 +82,37 @@ public class Engine extends Thread {
         try {
             while(true) {
                 listener.status("Locating Server");
-                // find out the TCP port
-                HttpURLConnection con = (HttpURLConnection)new URL(hudsonUrl).openConnection();
-                con.connect();
-                String port = con.getHeaderField("X-Hudson-JNLP-Port");
-                if(con.getResponseCode()!=200) {
-                    listener.error(new Exception(hudsonUrl+" is invalid: "+con.getResponseCode()+" "+con.getResponseMessage()));
-                    return;
+                Exception firstError=null;
+                String port=null;
+
+                for (URL url : candidateUrls) {
+                    String s = url.toExternalForm();
+                    if(!s.endsWith("/"))    s+='/';
+                    URL salURL = new URL(s+"tcpSlaveAgentListener/");
+
+                    // find out the TCP port
+                    HttpURLConnection con = (HttpURLConnection)salURL.openConnection();
+                    con.connect();
+                    port = con.getHeaderField("X-Hudson-JNLP-Port");
+                    if(con.getResponseCode()!=200) {
+                        if(firstError==null)
+                            firstError = new Exception(salURL+" is invalid: "+con.getResponseCode()+" "+con.getResponseMessage());
+                        continue;
+                    }
+                    if(port ==null) {
+                        if(firstError==null)
+                            firstError = new Exception(url+" is not Hudson");
+                        continue;
+                    }
+
+                    // this URL works. From now on, only try this URL
+                    hudsonUrl = url;
+                    candidateUrls = Collections.singletonList(hudsonUrl);
+                    break;
                 }
-                if(port ==null) {
-                    listener.error(new Exception(hudsonUrl+" is not Hudson: "));
+
+                if(firstError!=null) {
+                    listener.error(firstError);
                     return;
                 }
 
@@ -106,7 +144,7 @@ public class Engine extends Thread {
      * Connects to TCP slave port, with a few retries.
      */
     private Socket connect(String port) throws IOException, InterruptedException {
-        String host = this.host;
+        String host = this.hudsonUrl.getHost();
 
         if(tunnel!=null) {
             String[] tokens = tunnel.split(":");
@@ -138,7 +176,7 @@ public class Engine extends Thread {
         while(true) {
             Thread.sleep(1000*10);
             try {
-                HttpURLConnection con = (HttpURLConnection)new URL(hudsonUrl).openConnection();
+                HttpURLConnection con = (HttpURLConnection)hudsonUrl.openConnection();
                 con.connect();
                 if(con.getResponseCode()==200)
                     return;

remoting/src/main/java/hudson/remoting/jnlp/Main.java

@@ -9,6 +9,8 @@ import java.util.logging.Logger;
 import java.util.logging.Level;
 import java.util.List;
 import java.util.ArrayList;
+import java.net.URL;
+import java.io.IOException;
 
 import hudson.remoting.Engine;
 import hudson.remoting.EngineListener;
@@ -34,6 +36,10 @@ public class Main {
     public boolean headlessMode = Boolean.getBoolean("hudson.agent.headless")
                     || Boolean.getBoolean("hudson.webstart.headless");
 
+    @Option(name="-url",
+            usage="Specify the Hudson root URLs to connect to.")
+    public final List<URL> urls = new ArrayList<URL>();
+
     /**
      * 4 mandatory parameters.
      * Host name (deprecated), Hudson URL, secret key, and slave name.
@@ -41,7 +47,7 @@ public class Main {
     @Argument
     public final List<String> args = new ArrayList<String>();
 
-    public static void main(String[] args) {
+    public static void main(String[] args) throws IOException {
         // see http://forum.java.sun.com/thread.jspa?threadID=706976&tstart=0
         // not sure if this is the cause, but attempting to fix
         // https://hudson.dev.java.net/issues/show_bug.cgi?id=310
@@ -57,11 +63,13 @@ public class Main {
         CmdLineParser p = new CmdLineParser(m);
         try {
             p.parseArgument(args);
-            if(m.args.size()!=4)
-                throw new CmdLineException("four arguments required");
+            if(m.args.size()!=2)
+                throw new CmdLineException("two arguments required");
+            if(m.urls.isEmpty())
+                throw new CmdLineException("At least one -url option is required.");
         } catch (CmdLineException e) {
             System.err.println(e.getMessage());
-            System.err.println("java -jar jnlp-agent.jar [options...] <host> <hudson URL> <secret key> <slave name>");
+            System.err.println("java -jar slave.jar [options...] <secret key> <slave name>");
             p.printUsage(System.err);
             return;
         }
@@ -69,10 +77,10 @@ public class Main {
         m.main();
     }
 
-    public void main() {
+    public void main() throws IOException {
         Engine engine = new Engine(
                 headlessMode ? new CuiListener() : new GuiListener(),
-                args.get(0), args.get(1), args.get(2), args.get(3));
+                urls, args.get(0), args.get(1));
         if(tunnel!=null)
             engine.setTunnel(tunnel);
         engine.start();