Hang in Util.createSocket() if host is unreachable and a timeout is specified

[NickstaDB]

I was debugging some code that uses JSch as follows:

Session session = new JSch().getSession(username, host, port);
session.setPassword(password);
session.connect(5000);

When the target host was unreachable, the program hung after main() had returned.

This is down to the createSocket() method of the class com.jcraft.jsch.Util.

https://github.com/mwiede/jsch/blob/master/src/main/java/com/jcraft/jsch/Util.java#L371

With a non-zero timeout, the method uses a thread to call new Socket(host, port), and the Thread.join(timeout) method is used to handle the timeout. If a connection is not established within the given time then Thread.interrupt() is called in an attempt to terminate the thread cleanly. This can lead to a hang, presumably until a default timeout occurs. I'm not sure if it differs between environments or Java versions but in my case (Windows, OpenJDK 19) the program hung for about 15 seconds after main() had returned.

The Util.createSocket() method also creates a bunch of seemingly redundant variables on the stack and heap when the timeout is non-zero.

The only reason I can think of for handling the timeout using a thread is that this is legacy code for Java < 1.4. A timeout parameter was introduced to the Socket.connect() method in Java 1.4. Assuming this is legacy code for <1.4 support, it's redundant, because the Util.createSocket() method also uses a lambda for the thread function and lambdas weren't supported until Java 8.

The Util.createSocket() method can therefore be replaced with something like the following - no need to use a thread or create a bunch of redundant variables:

static Socket createSocket(String host, int port, int timeout) throws JSchException {
    try {
        Socket socket = socket = new Socket();
        socket.connect(new InetSocketAddress(host, port), timeout);
        return socket;
    } catch(SocketTimeoutException ex) {
        throw new JSchException("timeout: " + ex, ex);
    } catch(Exception ex) {
        throw new JSchException("socket is not established: " + ex, ex);
    }
}

I'm not sure if there's some other reason for using a thread, or if there are edge cases where my suggestion wouldn't work. Happy to submit a PR to clean this method up.

[norrisjeremy]

Hi @NickstaDB,

Please see #451 that I have opened.

Thanks,
Jeremy

[NickstaDB]

Nice, looks good, thanks!