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>
diff --git a/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestBase.java b/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestBase.java
index 2d284cf..3784741 100644
--- a/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestBase.java
+++ b/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.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.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 @@
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
diff --git a/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java b/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java
index 250a138..ab8e0c1 100644
--- a/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java
+++ b/org.eclipse.jgit.junit.ssh/src/org/eclipse/jgit/junit/ssh/SshTestGitServer.java
@@ -12,6 +12,8 @@
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.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 @@
*/
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 @@
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 @@
}
}
+
+ /**
+ * 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();
+ }
+ }
}
diff --git a/org.eclipse.jgit.ssh.jsch/src/org/eclipse/jgit/transport/JschSession.java b/org.eclipse.jgit.ssh.jsch/src/org/eclipse/jgit/transport/JschSession.java
index 300e03d..858bdf3 100644
--- a/org.eclipse.jgit.ssh.jsch/src/org/eclipse/jgit/transport/JschSession.java
+++ b/org.eclipse.jgit.ssh.jsch/src/org/eclipse/jgit/transport/JschSession.java
@@ -198,7 +198,7 @@
@Override
public int exitValue() {
if (isRunning())
- throw new IllegalStateException();
+ throw new IllegalThreadStateException();
return channel.getExitStatus();
}
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 8cbcb0f..899a799 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -621,6 +621,7 @@
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}
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 782a3f8..2976ab6 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -649,6 +649,7 @@
/***/ public String sourceRefNotSpecifiedForRefspec;
/***/ public String squashCommitNotUpdatingHEAD;
/***/ public String sshCommandFailed;
+ /***/ public String sshCommandTimeout;
/***/ public String sslFailureExceptionMessage;
/***/ public String sslFailureInfo;
/***/ public String sslFailureCause;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/SshSupport.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/SshSupport.java
index a151cd3..e297041 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/SshSupport.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/SshSupport.java
@@ -11,6 +11,7 @@
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 @@
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 @@
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 @@
}
}
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 @@
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;
+ }
}
