Allow retrying connecting SshSession in case of an exception

Connecting to an SshSession may fail due to different reasons. Jsch for 
example often throws an com.jcraft.jsch.JschException: verify: false.[1]
The issue is still not fixed in JSch 0.1.51.
 
In such a case it is worth retrying to connect. The number of connection
attempts can be configured using ssh_config parameter
"ConnectionAttempts" [2].

Don't retry if the user canceled authentication.

[1] http://sourceforge.net/p/jsch/bugs/58/
[2] http://linux.die.net/man/5/ssh_config

Bug: 437656
Change-Id: I6dd2a3786b7d3f15f5a46821d8edac987a57e381
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

6 files changed, 103 insertions, 12 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java
index 5836db1e60..8ec39bf56d 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008, Google Inc.
+ * Copyright (C) 2008, 2014 Google Inc.
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -96,6 +96,7 @@ public class OpenSshConfigTest extends RepositoryTestCase {
 		assertEquals("repo.or.cz", h.getHostName());
 		assertEquals("jex_junit", h.getUser());
 		assertEquals(22, h.getPort());
+		assertEquals(1, h.getConnectionAttempts());
 		assertNull(h.getIdentityFile());
 	}
 
@@ -249,4 +250,35 @@ public class OpenSshConfigTest extends RepositoryTestCase {
 		assertNotNull(h);
 		assertTrue(h.isBatchMode());
 	}
+
+	@Test
+	public void testAlias_ConnectionAttemptsDefault() throws Exception {
+		final Host h = osc.lookup("orcz");
+		assertNotNull(h);
+		assertEquals(1, h.getConnectionAttempts());
+	}
+
+	@Test
+	public void testAlias_ConnectionAttempts() throws Exception {
+		config("Host orcz\n" + "\tConnectionAttempts 5\n");
+		final Host h = osc.lookup("orcz");
+		assertNotNull(h);
+		assertEquals(5, h.getConnectionAttempts());
+	}
+
+	@Test
+	public void testAlias_invalidConnectionAttempts() throws Exception {
+		config("Host orcz\n" + "\tConnectionAttempts -1\n");
+		final Host h = osc.lookup("orcz");
+		assertNotNull(h);
+		assertEquals(1, h.getConnectionAttempts());
+	}
+
+	@Test
+	public void testAlias_badConnectionAttempts() throws Exception {
+		config("Host orcz\n" + "\tConnectionAttempts xxx\n");
+		final Host h = osc.lookup("orcz");
+		assertNotNull(h);
+		assertEquals(1, h.getConnectionAttempts());
+	}
 }
diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters
index 87a931c879..56c9ee68a0 100644
--- a/org.eclipse.jgit/.settings/.api_filters
+++ b/org.eclipse.jgit/.settings/.api_filters
@@ -7,6 +7,12 @@
                 <message_argument value="3.2.0"/>
             </message_arguments>
         </filter>
+        <filter comment="minor addition" id="924844039">
+            <message_arguments>
+                <message_argument value="3.4.0"/>
+                <message_argument value="3.4.0"/>
+            </message_arguments>
+        </filter>
     </resource>
     <resource path="src/org/eclipse/jgit/transport/TransportHttp.java" type="org.eclipse.jgit.transport.TransportHttp">
         <filter comment="Method is only used by implementers of TransportHttp's API, minor version are allowed to break implementer API according to OSGi semantic versioning (http://www.osgi.org/wiki/uploads/Links/SemanticVersioning.pdf)" id="338792546">
diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index fd5801e6a5..5b12a0c688 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -508,6 +508,7 @@ transportProtoHTTP=HTTP
 transportProtoLocal=Local Git Repository
 transportProtoSFTP=SFTP
 transportProtoSSH=SSH
+transportSSHRetryInterrupt=Interrupted while waiting for retry
 treeEntryAlreadyExists=Tree entry "{0}" already exists.
 treeFilterMarkerTooManyFilters=Too many markTreeFilters passed, maximum number is {0} (passed {1})
 treeIteratorDoesNotSupportRemove=TreeIterator does not support remove()
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index 8acfb54b85..f075db3e57 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -570,6 +570,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String transportProtoLocal;
 	/***/ public String transportProtoSFTP;
 	/***/ public String transportProtoSSH;
+	/***/ public String transportSSHRetryInterrupt;
 	/***/ public String treeEntryAlreadyExists;
 	/***/ public String treeFilterMarkerTooManyFilters;
 	/***/ public String treeIteratorDoesNotSupportRemove;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/JschConfigSessionFactory.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/JschConfigSessionFactory.java
index 65e9d4dc55..308741e935 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/JschConfigSessionFactory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/JschConfigSessionFactory.java
@@ -110,7 +110,7 @@ public abstract class JschConfigSessionFactory extends SshSessionFactory {
 					pass, host, port, hc);
 
 			int retries = 0;
-			while (!session.isConnected() && retries < 3) {
+			while (!session.isConnected()) {
 				try {
 					retries++;
 					session.connect(tms);
@@ -120,16 +120,30 @@ public abstract class JschConfigSessionFactory extends SshSessionFactory {
 					// Make sure our known_hosts is not outdated
 					knownHosts(getJSch(hc, fs), fs);
 
-					// if authentication failed maybe credentials changed at the
-					// remote end therefore reset credentials and retry
-					if (credentialsProvider != null && e.getCause() == null
-							&& e.getMessage().equals("Auth fail") //$NON-NLS-1$
-							&& retries < 3) {
-						credentialsProvider.reset(uri);
-						session = createSession(credentialsProvider, fs, user,
-								pass, host, port, hc);
-					} else {
+					if (isAuthenticationCanceled(e)) {
+						throw e;
+					} else if (isAuthenticationFailed(e)
+							&& credentialsProvider != null) {
+						// if authentication failed maybe credentials changed at
+						// the remote end therefore reset credentials and retry
+						if (retries < 3) {
+							credentialsProvider.reset(uri);
+							session = createSession(credentialsProvider, fs,
+									user, pass, host, port, hc);
+						} else
+							throw e;
+					} else if (retries >= hc.getConnectionAttempts()) {
 						throw e;
+					} else {
+						try {
+							Thread.sleep(1000);
+							session = createSession(credentialsProvider, fs,
+									user, pass, host, port, hc);
+						} catch (InterruptedException e1) {
+							throw new TransportException(
+									JGitText.get().transportSSHRetryInterrupt,
+									e1);
+						}
 					}
 				}
 			}
@@ -147,6 +161,14 @@ public abstract class JschConfigSessionFactory extends SshSessionFactory {
 
 	}
 
+	private static boolean isAuthenticationFailed(JSchException e) {
+		return e.getCause() == null && e.getMessage().equals("Auth fail"); //$NON-NLS-1$
+	}
+
+	private static boolean isAuthenticationCanceled(JSchException e) {
+		return e.getCause() == null && e.getMessage().equals("Auth cancel"); //$NON-NLS-1$
+	}
+
 	private Session createSession(CredentialsProvider credentialsProvider,
 			FS fs, String user, final String pass, String host, int port,
 			final OpenSshConfig.Host hc) throws JSchException {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/OpenSshConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/OpenSshConfig.java
index 01716da70a..38f3a2ac77 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/OpenSshConfig.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/OpenSshConfig.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008-2009, Google Inc.
+ * Copyright (C) 2008, 2014, Google Inc.
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -148,6 +148,8 @@ public class OpenSshConfig {
 			h.user = OpenSshConfig.userName();
 		if (h.port == 0)
 			h.port = OpenSshConfig.SSH_PORT;
+		if (h.connectionAttempts == 0)
+			h.connectionAttempts = 1;
 		h.patternsApplied = true;
 		return h;
 	}
@@ -244,6 +246,18 @@ public class OpenSshConfig {
 				for (final Host c : current)
 					if (c.strictHostKeyChecking == null)
 						c.strictHostKeyChecking = value;
+			} else if (StringUtils.equalsIgnoreCase(
+					"ConnectionAttempts", keyword)) { //$NON-NLS-1$
+				try {
+					final int connectionAttempts = Integer.parseInt(dequote(argValue));
+					if (connectionAttempts > 0) {
+						for (final Host c : current)
+							if (c.connectionAttempts == 0)
+								c.connectionAttempts = connectionAttempts;
+					}
+				} catch (NumberFormatException nfe) {
+					// ignore bad values
+				}
 			}
 		}
 
@@ -331,6 +345,8 @@ public class OpenSshConfig {
 
 		String strictHostKeyChecking;
 
+		int connectionAttempts;
+
 		void copyFrom(final Host src) {
 			if (hostName == null)
 				hostName = src.hostName;
@@ -346,6 +362,8 @@ public class OpenSshConfig {
 				batchMode = src.batchMode;
 			if (strictHostKeyChecking == null)
 				strictHostKeyChecking = src.strictHostKeyChecking;
+			if (connectionAttempts == 0)
+				connectionAttempts = src.connectionAttempts;
 		}
 
 		/**
@@ -402,5 +420,16 @@ public class OpenSshConfig {
 		public boolean isBatchMode() {
 			return batchMode != null && batchMode.booleanValue();
 		}
+
+		/**
+		 * @return the number of tries (one per second) to connect before
+		 *         exiting. The argument must be an integer. This may be useful
+		 *         in scripts if the connection sometimes fails. The default is
+		 *         1.
+		 * @since 3.4
+		 */
+		public int getConnectionAttempts() {
+			return connectionAttempts;
+		}
 	}
 }