Fix JSchProcess.waitFor() with time-out

SshSupport.runSshCommand() had a comment that wait with time-out
could not be used because JSchProcess.exitValue() threw the wrong
unchecked exception when the process was still running.

Fix this and make JSchProcess.exitValue() throw the right exception,
then wait with a time-out in SshSupport.

The Apache sshd client's SshdExecProcess has always used the correct
IllegalThreadStateException.

Add tests for SshSupport.runCommand().

Change-Id: Id30893174ae8be3b9a16119674049337b0cf4381
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestBase.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2018, Thomas Wolf <thomas.wolf@paranor.ch> and others
+ * Copyright (C) 2018, 2020 Thomas Wolf <thomas.wolf@paranor.ch> and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -14,6 +14,7 @@ import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeTrue;
 
@@ -22,9 +23,14 @@ import java.io.IOException;
 import java.nio.file.Files;
 import java.util.List;
 import java.util.Locale;
+import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jgit.api.errors.TransportException;
+import org.eclipse.jgit.errors.CommandFailedException;
 import org.eclipse.jgit.transport.CredentialItem;
+import org.eclipse.jgit.transport.URIish;
+import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.SshSupport;
 import org.junit.Test;
 import org.junit.experimental.theories.DataPoints;
 import org.junit.experimental.theories.Theory;
@@ -67,10 +73,45 @@ public abstract class SshTestBase extends SshTestHarness {
 		defaultCloneDir = new File(getTemporaryDirectory(), "cloned");
 	}
 
-	@Test(expected = TransportException.class)
+	@Test
 	public void testSshWithoutConfig() throws Exception {
-		cloneWith("ssh://" + TEST_USER + "@localhost:" + testPort
-				+ "/doesntmatter", defaultCloneDir, null);
+		assertThrows(TransportException.class,
+				() -> cloneWith("ssh://" + TEST_USER + "@localhost:" + testPort
+						+ "/doesntmatter", defaultCloneDir, null));
+	}
+
+	@Test
+	public void testSingleCommand() throws Exception {
+		installConfig("IdentityFile " + privateKey1.getAbsolutePath());
+		String command = SshTestGitServer.ECHO_COMMAND + " 1 without timeout";
+		long start = System.nanoTime();
+		String reply = SshSupport.runSshCommand(
+				new URIish("ssh://" + TEST_USER + "@localhost:" + testPort),
+				null, FS.DETECTED, command, 0); // 0 == no timeout
+		long elapsed = System.nanoTime() - start;
+		assertEquals(command, reply);
+		// Now that we have an idea how long this takes on the test
+		// infrastructure, try again with a timeout.
+		command = SshTestGitServer.ECHO_COMMAND + " 1 expecting no timeout";
+		// Still use a generous timeout.
+		int timeout = 10 * ((int) TimeUnit.NANOSECONDS.toSeconds(elapsed) + 1);
+		reply = SshSupport.runSshCommand(
+				new URIish("ssh://" + TEST_USER + "@localhost:" + testPort),
+				null, FS.DETECTED, command, timeout);
+		assertEquals(command, reply);
+	}
+
+	@Test
+	public void testSingleCommandWithTimeoutExpired() throws Exception {
+		installConfig("IdentityFile " + privateKey1.getAbsolutePath());
+		String command = SshTestGitServer.ECHO_COMMAND + " 2 EXPECTING TIMEOUT";
+
+		CommandFailedException e = assertThrows(CommandFailedException.class,
+				() -> SshSupport.runSshCommand(new URIish(
+						"ssh://" + TEST_USER + "@localhost:" + testPort), null,
+						FS.DETECTED, command, 1));
+		assertTrue(e.getMessage().contains(command));
+		assertTrue(e.getMessage().contains("time"));
 	}
 
 	@Test

org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java

@@ -12,6 +12,8 @@ package org.eclipse.jgit.junit.ssh;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.security.GeneralSecurityException;
@@ -22,6 +24,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
+import java.util.concurrent.TimeUnit;
 
 import org.apache.sshd.common.NamedResource;
 import org.apache.sshd.common.PropertyResolver;
@@ -67,6 +70,16 @@ import org.eclipse.jgit.transport.UploadPack;
  */
 public class SshTestGitServer {
 
+	/**
+	 * Simple echo test command. Replies with the command string as passed. If
+	 * of the form "echo [int] anything", takes the integer value as a delay in
+	 * seconds before replying, which may be useful to test various
+	 * timeout-related things.
+	 *
+	 * @since 5.9
+	 */
+	public static final String ECHO_COMMAND = "echo";
+
 	@NonNull
 	protected final String testUser;
 
@@ -166,6 +179,8 @@ public class SshTestGitServer {
 				return new GitUploadPackCommand(command, executorService);
 			} else if (command.startsWith(RemoteConfig.DEFAULT_RECEIVE_PACK)) {
 				return new GitReceivePackCommand(command, executorService);
+			} else if (command.startsWith(ECHO_COMMAND)) {
+				return new EchoCommand(command, executorService);
 			}
 			return new UnknownCommand(command);
 		});
@@ -475,4 +490,52 @@ public class SshTestGitServer {
 		}
 
 	}
+
+	/**
+	 * Simple echo command that echoes back the command string. If the first
+	 * argument is a positive integer, it's taken as a delay (in seconds) before
+	 * replying. Assumes UTF-8 character encoding.
+	 */
+	private static class EchoCommand extends AbstractCommandSupport {
+
+		protected EchoCommand(String command,
+				CloseableExecutorService executorService) {
+			super(command, ThreadUtils.noClose(executorService));
+		}
+
+		@Override
+		public void run() {
+			String[] parts = getCommand().split(" ");
+			int timeout = 0;
+			if (parts.length >= 2) {
+				try {
+					timeout = Integer.parseInt(parts[1]);
+				} catch (NumberFormatException e) {
+					// No timeout.
+				}
+				if (timeout > 0) {
+					try {
+						Thread.sleep(TimeUnit.SECONDS.toMillis(timeout));
+					} catch (InterruptedException e) {
+						// Ignore.
+					}
+				}
+			}
+			try {
+				doEcho(getCommand(), getOutputStream());
+				onExit(0);
+			} catch (IOException e) {
+				log.warn(
+						MessageFormat.format("Could not run {0}", getCommand()),
+						e);
+				onExit(-1, e.toString());
+			}
+		}
+
+		private void doEcho(String text, OutputStream stream)
+				throws IOException {
+			stream.write(text.getBytes(StandardCharsets.UTF_8));
+			stream.flush();
+		}
+	}
 }

org.eclipse.jgit.ssh.jsch/src/org/eclipse/jgit/transport/JschSession.java

@@ -198,7 +198,7 @@ public class JschSession implements RemoteSession {
 		@Override
 		public int exitValue() {
 			if (isRunning())
-				throw new IllegalStateException();
+				throw new IllegalThreadStateException();
 			return channel.getExitStatus();
 		}
 

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -621,6 +621,7 @@ sourceRefDoesntResolveToAnyObject=Source ref {0} doesn''t resolve to any object.
 sourceRefNotSpecifiedForRefspec=Source ref not specified for refspec: {0}
 squashCommitNotUpdatingHEAD=Squash commit -- not updating HEAD
 sshCommandFailed=Execution of ssh command ''{0}'' failed with error ''{1}''
+sshCommandTimeout=Execution of ssh command ''{0}'' timed out after {1} seconds
 sslFailureExceptionMessage=Secure connection to {0} could not be established because of SSL problems
 sslFailureInfo=A secure connection to {0} could not be established because the server''s certificate could not be validated.
 sslFailureCause=SSL reported: {0}

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -649,6 +649,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String sourceRefNotSpecifiedForRefspec;
 	/***/ public String squashCommitNotUpdatingHEAD;
 	/***/ public String sshCommandFailed;
+	/***/ public String sshCommandTimeout;
 	/***/ public String sslFailureExceptionMessage;
 	/***/ public String sslFailureInfo;
 	/***/ public String sslFailureCause;

org.eclipse.jgit/src/org/eclipse/jgit/util/SshSupport.java

@@ -11,6 +11,7 @@ package org.eclipse.jgit.util;
 
 import java.io.IOException;
 import java.text.MessageFormat;
+import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.errors.CommandFailedException;
@@ -61,11 +62,21 @@ public class SshSupport {
 		CommandFailedException failure = null;
 		@SuppressWarnings("resource")
 		MessageWriter stderr = new MessageWriter();
+		@SuppressWarnings("resource")
+		MessageWriter stdout = new MessageWriter();
 		String out;
-		try (MessageWriter stdout = new MessageWriter()) {
+		try {
+			long start = System.nanoTime();
 			session = SshSessionFactory.getInstance().getSession(sshUri,
 					provider, fs, 1000 * timeout);
-			process = session.exec(command, 0);
+			int commandTimeout = timeout;
+			if (timeout > 0) {
+				commandTimeout = checkTimeout(command, timeout, start);
+			}
+			process = session.exec(command, commandTimeout);
+			if (timeout > 0) {
+				commandTimeout = checkTimeout(command, timeout, start);
+			}
 			errorThread = new StreamCopyThread(process.getErrorStream(),
 					stderr.getRawStream());
 			errorThread.start();
@@ -73,9 +84,15 @@ public class SshSupport {
 					stdout.getRawStream());
 			outThread.start();
 			try {
-				// waitFor with timeout has a bug - JSch' exitValue() throws the
-				// wrong exception type :(
-				if (process.waitFor() == 0) {
+				boolean finished = false;
+				if (timeout <= 0) {
+					process.waitFor();
+					finished = true;
+				} else {
+					finished = process.waitFor(commandTimeout,
+							TimeUnit.SECONDS);
+				}
+				if (finished) {
 					out = stdout.toString();
 				} else {
 					out = null; // still running after timeout
@@ -103,15 +120,26 @@ public class SshSupport {
 				}
 			}
 			if (process != null) {
-				if (process.exitValue() != 0) {
-					failure = new CommandFailedException(process.exitValue(),
+				try {
+					if (process.exitValue() != 0) {
+						failure = new CommandFailedException(
+								process.exitValue(),
+								MessageFormat.format(
+										JGitText.get().sshCommandFailed,
+										command, stderr.toString()));
+					}
+					// It was successful after all
+					out = stdout.toString();
+				} catch (IllegalThreadStateException e) {
+					failure = new CommandFailedException(0,
 							MessageFormat.format(
-							JGitText.get().sshCommandFailed, command,
-							stderr.toString()));
+									JGitText.get().sshCommandTimeout, command,
+									Integer.valueOf(timeout)));
 				}
 				process.destroy();
 			}
 			stderr.close();
+			stdout.close();
 			if (session != null) {
 				SshSessionFactory.getInstance().releaseSession(session);
 			}
@@ -122,4 +150,17 @@ public class SshSupport {
 		return out;
 	}
 
+	private static int checkTimeout(String command, int timeout, long since)
+			throws CommandFailedException {
+		long elapsed = System.nanoTime() - since;
+		int newTimeout = timeout
+				- (int) TimeUnit.NANOSECONDS.toSeconds(elapsed);
+		if (newTimeout <= 0) {
+			// All time used up for connecting the session
+			throw new CommandFailedException(0,
+					MessageFormat.format(JGitText.get().sshCommandTimeout,
+							command, Integer.valueOf(timeout)));
+		}
+		return newTimeout;
+	}
 }