Merge pull request #2156 from kzantow/JENKINS-33557-tcp-timeouts-duri…

…ng-install [JENKINS-33557] - connectivity checks may cause setup wizard to hang for a long time
jenkinsci · Mar 23, 2016 · ee5c1a3 · ee5c1a3
2 parents 6a74607 + 447a9b7
commit ee5c1a3
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 29 deletions.
diff --git a/core/src/main/java/hudson/ProxyConfiguration.java b/core/src/main/java/hudson/ProxyConfiguration.java
@@ -74,6 +74,12 @@
  * @see jenkins.model.Jenkins#proxy
  */
 public final class ProxyConfiguration extends AbstractDescribableImpl<ProxyConfiguration> implements Saveable, Serializable {
+    /**
+     * Holds a default TCP connect timeout set on all connections returned from this class,
+     * note this is value is in milliseconds, it's passed directly to {@link URLConnection#setConnectTimeout(int)}
+     */
+    private static final int DEFAULT_CONNECT_TIMEOUT_MILLIS = Integer.getInteger("hudson.ProxyConfiguration.DEFAULT_CONNECT_TIMEOUT_MILLIS", 20 * 1000);
+
     public final String name;
     public final int port;
 
@@ -218,22 +224,29 @@ public static ProxyConfiguration load() throws IOException {
      */
     public static URLConnection open(URL url) throws IOException {
         final ProxyConfiguration p = get();
-        if(p==null)
-            return url.openConnection();
-
-        URLConnection con = url.openConnection(p.createProxy(url.getHost()));
-        if(p.getUserName()!=null) {
-            // Add an authenticator which provides the credentials for proxy authentication
-            Authenticator.setDefault(new Authenticator() {
-                @Override
-                public PasswordAuthentication getPasswordAuthentication() {
-                    if (getRequestorType()!=RequestorType.PROXY)    return null;
-                    return new PasswordAuthentication(p.getUserName(),
-                            p.getPassword().toCharArray());
-                }
-            });
+
+        URLConnection con;
+        if(p==null) {
+            con = url.openConnection();
+        } else {
+            con = url.openConnection(p.createProxy(url.getHost()));
+            if(p.getUserName()!=null) {
+                // Add an authenticator which provides the credentials for proxy authentication
+                Authenticator.setDefault(new Authenticator() {
+                    @Override
+                    public PasswordAuthentication getPasswordAuthentication() {
+                        if (getRequestorType()!=RequestorType.PROXY)    return null;
+                        return new PasswordAuthentication(p.getUserName(),
+                                p.getPassword().toCharArray());
+                    }
+                });
+            }
         }
-
+
+        if(DEFAULT_CONNECT_TIMEOUT_MILLIS > 0) {
+            con.setConnectTimeout(DEFAULT_CONNECT_TIMEOUT_MILLIS);
+        }
+
         if (JenkinsJVM.isJenkinsJVM()) { // this code may run on a slave
             decorate(con);
         }
@@ -329,7 +342,7 @@ public FormValidation doValidateProxy(
             GetMethod method = null;
             try {
                 method = new GetMethod(testUrl);
-                method.getParams().setParameter("http.socket.timeout", new Integer(30 * 1000));
+                method.getParams().setParameter("http.socket.timeout", DEFAULT_CONNECT_TIMEOUT_MILLIS > 0 ? DEFAULT_CONNECT_TIMEOUT_MILLIS : new Integer(30 * 1000));
 
                 HttpClient client = new HttpClient();
                 if (Util.fixEmptyAndTrim(name) != null) {

diff --git a/core/src/main/java/hudson/model/UpdateCenter.java b/core/src/main/java/hudson/model/UpdateCenter.java
@@ -77,6 +77,8 @@
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.net.HttpRetryException;
+import java.net.HttpURLConnection;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLConnection;
@@ -1132,7 +1134,16 @@ public String getPluginRepositoryBaseUrl() {
 
         private void testConnection(URL url) throws IOException {
             try {
-                Util.copyStreamAndClose(ProxyConfiguration.open(url).getInputStream(),new NullOutputStream());
+                URLConnection connection = (URLConnection) ProxyConfiguration.open(url);
+
+                if(connection instanceof HttpURLConnection) {
+                    int responseCode = ((HttpURLConnection)connection).getResponseCode();
+                    if(HttpURLConnection.HTTP_OK != responseCode) {
+                        throw new HttpRetryException("Invalid response code (" + responseCode + ") from URL: " + url, responseCode);
+                    }
+                } else {
+                    Util.copyStreamAndClose(connection.getInputStream(),new NullOutputStream());
+                }
             } catch (SSLHandshakeException e) {
                 if (e.getMessage().contains("PKIX path building failed"))
                    // fix up this crappy error message from JDK
@@ -1315,22 +1326,29 @@ public void run() {
                 return;
             }
             LOGGER.fine("Doing a connectivity check");
+            Future<?> internetCheck = null;
             try {
-                String connectionCheckUrl = site.getConnectionCheckUrl();
+                final String connectionCheckUrl = site.getConnectionCheckUrl();
                 if (connectionCheckUrl!=null) {
                     connectionStates.put(ConnectionStatus.INTERNET, ConnectionStatus.CHECKING);
                     statuses.add(Messages.UpdateCenter_Status_CheckingInternet());
-                    try {
-                        config.checkConnection(this, connectionCheckUrl);
-                    } catch (Exception e) {
-                        if(e.getMessage().contains("Connection timed out")) {
-                            // Google can't be down, so this is probably a proxy issue
-                            connectionStates.put(ConnectionStatus.INTERNET, ConnectionStatus.FAILED);
-                            statuses.add(Messages.UpdateCenter_Status_ConnectionFailed(connectionCheckUrl));
-                            return;
+                    // Run the internet check in parallel
+                    internetCheck = updateService.submit(new Runnable() {
+                        @Override
+                        public void run() {
+                            try {
+                                config.checkConnection(ConnectionCheckJob.this, connectionCheckUrl);
+                            } catch (Exception e) {
+                                if(e.getMessage().contains("Connection timed out")) {
+                                    // Google can't be down, so this is probably a proxy issue
+                                    connectionStates.put(ConnectionStatus.INTERNET, ConnectionStatus.FAILED);
+                                    statuses.add(Messages.UpdateCenter_Status_ConnectionFailed(connectionCheckUrl));
+                                    return;
+                                }
+                            }
+                            connectionStates.put(ConnectionStatus.INTERNET, ConnectionStatus.OK);
                         }
-                    }
-                    connectionStates.put(ConnectionStatus.INTERNET, ConnectionStatus.OK);
+                    });
                 }
 
                 connectionStates.put(ConnectionStatus.UPDATE_SITE, ConnectionStatus.CHECKING);
@@ -1350,6 +1368,15 @@ public void run() {
                 statuses.add(Functions.printThrowable(e));
                 error = e;
             }
+
+            if(internetCheck != null) {
+                try {
+                    // Wait for internet check to complete
+                    internetCheck.get();
+                } catch (Exception e) {
+                    LOGGER.log(Level.WARNING, "Error completing internet connectivity check: " + e.getMessage(), e);
+                }
+            }
         }
 
         private void addStatus(UnknownHostException e) {

diff --git a/test/src/test/java/hudson/model/UpdateCenterConnectionStatusTest.java b/test/src/test/java/hudson/model/UpdateCenterConnectionStatusTest.java
@@ -99,7 +99,7 @@ public void test_states_internet_failed() {
         job.run();
 
         Assert.assertEquals(ConnectionStatus.FAILED, job.connectionStates.get(ConnectionStatus.INTERNET));
-        Assert.assertEquals(ConnectionStatus.UNCHECKED, job.connectionStates.get(ConnectionStatus.UPDATE_SITE));
+        Assert.assertEquals(ConnectionStatus.OK, job.connectionStates.get(ConnectionStatus.UPDATE_SITE));
     }
 
     @Test