Update javadoc for RemoteSession and SshSessionFactory

The timeout on RemoteSession.exec() cannot be a timeout for the
whole command. It can only be a timeout for setting up the process;
after that it's the application's responsibility to implement some
timeout for the execution of the command, for instance by calling
Process.waitFor(int, TimeUnit) or through other means.

Sessions returned by an SshSessionFactory are already connected and
authenticated -- they must be, because RemoteSession offers no
operations for connecting or authenticating a session.

Change the implementation of SshdExecProcess.waitFor() to wait
indefinitely. The original implementation used the timeout from
RemoteSession.exec() because of that erroneous javadoc.

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

org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/SshdSession.java

@@ -13,7 +13,6 @@ import static java.text.MessageFormat.format;
 
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.InterruptedIOException;
 import java.io.OutputStream;
 import java.time.Duration;
 import java.util.ArrayList;
@@ -134,28 +133,23 @@ public class SshdSession implements RemoteSession {
 	public Process exec(String commandName, int timeout) throws IOException {
 		@SuppressWarnings("resource")
 		ChannelExec exec = session.createExecChannel(commandName);
-		long timeoutMillis = TimeUnit.SECONDS.toMillis(timeout);
-		try {
-			if (timeout <= 0) {
+		if (timeout <= 0) {
+			try {
 				exec.open().verify();
-			} else {
-				long start = System.nanoTime();
-				exec.open().verify(timeoutMillis);
-				timeoutMillis -= TimeUnit.NANOSECONDS
-						.toMillis(System.nanoTime() - start);
+			} catch (IOException | RuntimeException e) {
+				exec.close(true);
+				throw e;
+			}
+		} else {
+			try {
+				exec.open().verify(TimeUnit.SECONDS.toMillis(timeout));
+			} catch (IOException | RuntimeException e) {
+				exec.close(true);
+				throw new IOException(format(SshdText.get().sshCommandTimeout,
+						commandName, Integer.valueOf(timeout)), e);
 			}
-		} catch (IOException | RuntimeException e) {
-			exec.close(true);
-			throw e;
-		}
-		if (timeout > 0 && timeoutMillis <= 0) {
-			// We have used up the whole timeout for opening the channel
-			exec.close(true);
-			throw new InterruptedIOException(
-					format(SshdText.get().sshCommandTimeout, commandName,
-							Integer.valueOf(timeout)));
 		}
-		return new SshdExecProcess(exec, commandName, timeoutMillis);
+		return new SshdExecProcess(exec, commandName);
 	}
 
 	/**
@@ -195,14 +189,10 @@ public class SshdSession implements RemoteSession {
 
 		private final ChannelExec channel;
 
-		private final long timeoutMillis;
-
 		private final String commandName;
 
-		public SshdExecProcess(ChannelExec channel, String commandName,
-				long timeoutMillis) {
+		public SshdExecProcess(ChannelExec channel, String commandName) {
 			this.channel = channel;
-			this.timeoutMillis = timeoutMillis > 0 ? timeoutMillis : -1L;
 			this.commandName = commandName;
 		}
 
@@ -223,7 +213,7 @@ public class SshdSession implements RemoteSession {
 
 		@Override
 		public int waitFor() throws InterruptedException {
-			if (waitFor(timeoutMillis, TimeUnit.MILLISECONDS)) {
+			if (waitFor(-1L, TimeUnit.MILLISECONDS)) {
 				return exitValue();
 			}
 			return -1;

org.eclipse.jgit/src/org/eclipse/jgit/transport/RemoteSession.java

@@ -4,7 +4,7 @@
  * Copyright (C) 2009, Google, Inc.
  * Copyright (C) 2009, JetBrains s.r.o.
  * Copyright (C) 2008, Robin Rosenberg <robin.rosenberg@dewire.com>
- * Copyright (C) 2008, Shawn O. Pearce <spearce@spearce.org> and others
+ * Copyright (C) 2008, 2020 Shawn O. Pearce <spearce@spearce.org> 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
@@ -18,26 +18,20 @@ package org.eclipse.jgit.transport;
 import java.io.IOException;
 
 /**
- * Create a remote "session" for executing remote commands.
- * <p>
- * Clients should subclass RemoteSession to create an alternate way for JGit to
- * execute remote commands. (The client application may already have this
- * functionality available.) Note that this class is just a factory for creating
- * remote processes. If the application already has a persistent connection to
- * the remote machine, RemoteSession may do nothing more than return a new
- * RemoteProcess when exec is called.
+ * An abstraction of a remote "session" for executing remote commands.
  */
 public interface RemoteSession {
+
 	/**
-	 * Generate a new remote process to execute the given command. This function
-	 * should also start execution and may need to create the streams prior to
-	 * execution.
+	 * Creates a new remote {@link Process} to execute the given command. The
+	 * returned process's streams exist and are connected, and execution of the
+	 * process is already started.
 	 *
 	 * @param commandName
 	 *            command to execute
 	 * @param timeout
-	 *            timeout value, in seconds, for command execution
-	 * @return a new remote process
+	 *            timeout value, in seconds, for creating the remote process
+	 * @return a new remote process, already started
 	 * @throws java.io.IOException
 	 *             may be thrown in several cases. For example, on problems
 	 *             opening input or output streams or on problems connecting or
@@ -48,7 +42,7 @@ public interface RemoteSession {
 	Process exec(String commandName, int timeout) throws IOException;
 
 	/**
-	 * Obtain an {@link FtpChannel} for performing FTP operations over this
+	 * Obtains an {@link FtpChannel} for performing FTP operations over this
 	 * {@link RemoteSession}. The default implementation returns {@code null}.
 	 *
 	 * @return the {@link FtpChannel}
@@ -59,7 +53,7 @@ public interface RemoteSession {
 	}
 
 	/**
-	 * Disconnect the remote session
+	 * Disconnects the remote session.
 	 */
 	void disconnect();
 }

org.eclipse.jgit/src/org/eclipse/jgit/transport/SshSessionFactory.java

@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2008, Robin Rosenberg <robin.rosenberg@dewire.com>
- * Copyright (C) 2008, Shawn O. Pearce <spearce@spearce.org> and others
+ * Copyright (C) 2008, 2020 Shawn O. Pearce <spearce@spearce.org> 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
@@ -27,12 +27,15 @@ import org.eclipse.jgit.util.SystemReader;
  * Different implementations of the session factory may be used to control
  * communicating with the end-user as well as reading their personal SSH
  * configuration settings, such as known hosts and private keys.
+ * </p>
  * <p>
- * A {@link org.eclipse.jgit.transport.RemoteSession} must be returned to the
- * factory that created it. Callers are encouraged to retain the
- * SshSessionFactory for the duration of the period they are using the Session.
+ * A {@link RemoteSession} must be returned to the factory that created it.
+ * Callers are encouraged to retain the SshSessionFactory for the duration of
+ * the period they are using the session.
+ * </p>
  */
 public abstract class SshSessionFactory {
+
 	private static SshSessionFactory INSTANCE = loadSshSessionFactory();
 
 	private static SshSessionFactory loadSshSessionFactory() {
@@ -45,10 +48,11 @@ public abstract class SshSessionFactory {
 	}
 
 	/**
-	 * Get the currently configured JVM-wide factory.
+	 * Gets the currently configured JVM-wide factory.
 	 * <p>
-	 * By default the factory will read from the user's <code>$HOME/.ssh</code>
-	 * and assume OpenSSH compatibility.
+	 * By default the factory will read from the user's {@code $HOME/.ssh} and
+	 * assume OpenSSH compatibility.
+	 * </p>
 	 *
 	 * @return factory the current factory for this JVM.
 	 */
@@ -57,11 +61,11 @@ public abstract class SshSessionFactory {
 	}
 
 	/**
-	 * Change the JVM-wide factory to a different implementation.
+	 * Changes the JVM-wide factory to a different implementation.
 	 *
 	 * @param newFactory
-	 *            factory for future sessions to be created through. If null the
-	 *            default factory will be restored.
+	 *            factory for future sessions to be created through; if
+	 *            {@code null} the default factory will be restored.
 	 */
 	public static void setInstance(SshSessionFactory newFactory) {
 		if (newFactory != null) {
@@ -85,26 +89,23 @@ public abstract class SshSessionFactory {
 	}
 
 	/**
-	 * Open (or reuse) a session to a host.
-	 * <p>
-	 * A reasonable UserInfo that can interact with the end-user (if necessary)
-	 * is installed on the returned session by this method.
-	 * <p>
-	 * The caller must connect the session by invoking <code>connect()</code> if
-	 * it has not already been connected.
+	 * Opens (or reuses) a session to a host. The returned session is connected
+	 * and authenticated and is ready for further use.
 	 *
 	 * @param uri
-	 *            URI information about the remote host
+	 *            URI of the remote host to connect to
 	 * @param credentialsProvider
-	 *            provider to support authentication, may be null.
+	 *            provider to support authentication, may be {@code null} if no
+	 *            user input for authentication is needed
 	 * @param fs
-	 *            the file system abstraction which will be necessary to perform
-	 *            certain file system operations.
+	 *            the file system abstraction to use for certain file
+	 *            operations, such as reading configuration files
 	 * @param tms
-	 *            Timeout value, in milliseconds.
-	 * @return a session that can contact the remote host.
+	 *            connection timeout for creating the session, in milliseconds
+	 * @return a connected and authenticated session for communicating with the
+	 *         remote host given by the {@code uri}
 	 * @throws org.eclipse.jgit.errors.TransportException
-	 *             the session could not be created.
+	 *             if the session could not be created
 	 */
 	public abstract RemoteSession getSession(URIish uri,
 			CredentialsProvider credentialsProvider, FS fs, int tms)
@@ -120,7 +121,7 @@ public abstract class SshSessionFactory {
 	public abstract String getType();
 
 	/**
-	 * Close (or recycle) a session to a host.
+	 * Closes (or recycles) a session to a host.
 	 *
 	 * @param session
 	 *            a session previously obtained from this factory's