Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The timeout interval from the client SftpClient cannot be configured on the current server. #371

Closed
beijishiqidu opened this issue May 16, 2023 · 3 comments · Fixed by #382
Assignees
Labels
bug An issue describing a bug in the code
Milestone

Comments

@beijishiqidu
Copy link

Description

I wrote a simple demo

The Server code:

public static void main(String[] args) throws Exception {
        SshServer sshd = SshServer.setUpDefaultServer();
        KeyPairProvider keyPair = new SimpleGeneratorHostKeyProvider();
        sshd.setKeyPairProvider(keyPair);
        sshd.setPort(9999);
        List<SubsystemFactory> namedFactoryList = new ArrayList<>();
        namedFactoryList.add(new SftpSubsystemFactory());
        sshd.setSubsystemFactories(namedFactoryList);

        sshd.setFileSystemFactory(new VirtualFileSystemFactory() {
            @Override
            public Path getUserHomeDir(SessionContext session) {
                return Paths.get("D:\\sshdhomepath");
            }
        });

        sshd.setPasswordAuthenticator(new MyPasswordAuthenticator());
        sshd.start();
        while (true) {
            Thread.sleep(60000);
        }
    }

    private static class MyPasswordAuthenticator implements PasswordAuthenticator {
        public boolean authenticate(String username, String password, ServerSession session)
            throws PasswordChangeRequiredException, AsyncAuthException {
            log.info("current session is = {}", session);

            return "sftp".equals(username) && "sftp".equals(password);

        }
    }

The Client code:

public static void main(String[] args) throws Exception {
        SshClient client = SshClient.setUpDefaultClient();
        client.start();
        try (ClientSession session = client.connect("sftp", "127.0.0.1", 9999).verify().getSession()) {
            session.addPasswordIdentity("sftp");
            if (session.auth().verify().isSuccess()) {
                for (int i = 0; i < 1000; i++) {
                    SftpClientFactory factory = new DefaultSftpClientFactory();
                    SftpClient sftpClient = factory.createSftpClient(session);
                    try (FileInputStream in = new FileInputStream("D:\\20230113\\202301131049.dump")) {
                        SftpClient.CloseableHandle handle = sftpClient.open("202301131049.dump" + i,
                            SftpClient.OpenMode.Write, SftpClient.OpenMode.Create);
                        int buff_size = 1024 * 1024;
                        byte[] src = new byte[buff_size];
                        int len;
                        long fileOffset = 0L;
                        while ((len = in.read(src)) != -1) {
                            sftpClient.write(handle, fileOffset, src, 0, len);
                            fileOffset += len;
                        }
                    } catch (Exception e) {
                        e.printStackTrace();
                    }
                }
                log.info("upload file success");
            } else {
                log.error("can not create session");
            }
            Thread.sleep(2000);
        }
        client.close();
    }

Motivation

I refer to hackers or other clients and deliberately do not close the connection.
The following code is used to run the sshd client,I didn't turn it off, and I tried 1,000 times.

SftpClient sftpClient = factory.createSftpClient(session);

So, When I went to trace the stack on the server side, I found that many of the SftpSubsystem objects were not closed.

sh-4.3$ jstack -l 39322 |grep -i sshd
"sshd-SftpSubsystem-37077-thread-1" #29814 daemon prio=5 os_prio=0 cpu=89.41ms elapsed=2.83s tid=0x00007fcf6400f800 nid=0x1226 waiting on condition [0x00007fcba4203000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-41846-thread-1" #29795 daemon prio=5 os_prio=0 cpu=2927.78ms elapsed=85.33s tid=0x00007fcf880fa000 nid=0x1137 waiting on condition [0x00007fcba4d0e000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-33601-thread-1" #29789 daemon prio=5 os_prio=0 cpu=2868.93ms elapsed=167.03s tid=0x00007fcf38190000 nid=0xfea waiting on condition [0x00007fcba4405000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-8083-thread-1" #29784 daemon prio=5 os_prio=0 cpu=2865.43ms elapsed=248.49s tid=0x00007fcf8801b000 nid=0xe9f waiting on condition [0x00007fcba3cfe000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-42628-thread-1" #29778 daemon prio=5 os_prio=0 cpu=2872.83ms elapsed=330.10s tid=0x00007fcf40102000 nid=0xdbc waiting on condition [0x00007fcba4607000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-8096-thread-1" #29773 daemon prio=5 os_prio=0 cpu=2937.80ms elapsed=412.09s tid=0x00007fcf300ca800 nid=0xc70 waiting on condition [0x00007fcba4809000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-44074-thread-1" #29768 daemon prio=5 os_prio=0 cpu=2847.39ms elapsed=494.56s tid=0x00007fcf38191800 nid=0xb25 waiting on condition [0x00007fce619e0000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-27927-thread-1" #29763 daemon prio=5 os_prio=0 cpu=2970.71ms elapsed=576.55s tid=0x00007fcf3001a800 nid=0x9da waiting on condition [0x00007fcba4102000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-7402-thread-1" #29739 daemon prio=5 os_prio=0 cpu=2908.31ms elapsed=658.80s tid=0x00007fcf6800d000 nid=0x8b1 waiting on condition [0x00007fcba4708000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-26637-thread-1" #29733 daemon prio=5 os_prio=0 cpu=2869.69ms elapsed=740.26s tid=0x00007fcf5c0e3000 nid=0x765 waiting on condition [0x00007fcba3bfd000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-43298-thread-1" #29728 daemon prio=5 os_prio=0 cpu=2874.68ms elapsed=822.75s tid=0x00007fcf38011800 nid=0x61a waiting on condition [0x00007fcba39fb000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-2696-thread-1" #29723 daemon prio=5 os_prio=0 cpu=2875.22ms elapsed=905.24s tid=0x00007fcf6400f000 nid=0x4cd waiting on condition [0x00007fce61de4000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-9495-thread-1" #29717 daemon prio=5 os_prio=0 cpu=2934.66ms elapsed=987.72s tid=0x00007fcf4000b000 nid=0x3e9 waiting on condition [0x00007fcba3f00000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-55726-thread-1" #29712 daemon prio=5 os_prio=0 cpu=2847.24ms elapsed=1069.84s tid=0x00007fcf40015800 nid=0x281 waiting on condition [0x00007fcba3afc000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-46430-thread-1" #29706 daemon prio=5 os_prio=0 cpu=2831.26ms elapsed=1151.76s tid=0x00007fcf64011000 nid=0x134 waiting on condition [0x00007fcba3dff000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-12464-thread-1" #29686 daemon prio=5 os_prio=0 cpu=2842.01ms elapsed=1233.30s tid=0x00007fcf380de800 nid=0xfeac waiting on condition [0x00007fcba37f9000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-47609-thread-1" #29679 daemon prio=5 os_prio=0 cpu=2993.85ms elapsed=1314.92s tid=0x00007fcf3c0ce000 nid=0xfdc9 waiting on condition [0x00007fcba38fa000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-63307-thread-1" #29674 daemon prio=5 os_prio=0 cpu=2592.63ms elapsed=1395.36s tid=0x00007fcf6800b000 nid=0xfc5f waiting on condition [0x00007fcba36f8000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-10647-thread-1" #29669 daemon prio=5 os_prio=0 cpu=2601.42ms elapsed=1477.21s tid=0x00007fcf88007800 nid=0xfad4 waiting on condition [0x00007fcba34f6000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-12760-thread-1" #29664 daemon prio=5 os_prio=0 cpu=2558.38ms elapsed=1559.21s tid=0x00007fcf6400e000 nid=0xf9b5 waiting on condition [0x00007fcba35f7000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SftpSubsystem-23564-thread-1" #29657 daemon prio=5 os_prio=0 cpu=2614.00ms elapsed=1641.23s tid=0x00007fcf7804c800 nid=0xf7e6 waiting on condition [0x00007fce622e9000]
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
"sshd-SshServer[146e3c90](port=33233)-nio2-thread-9" #470 daemon prio=5 os_prio=0 cpu=47688.11ms elapsed=298207.55s tid=0x00007fd044013000 nid=0xcad3 waiting on condition [0x00007fcf825ee000]
"sshd-SshServer[146e3c90](port=33233)-nio2-thread-8" #469 daemon prio=5 os_prio=0 cpu=47797.35ms elapsed=298207.56s tid=0x00007fcf4c01c800 nid=0xcad2 waiting on condition [0x00007fcf826ef000]

Alternatives considered

I tried to look for timeout-related configurations in SftpModuleProperties and CoreModuleProperties, but I didn't find any. So, I'd like to ask, do we currently support this capability?

Thank you very much

Additional context

No response

@tomaswolf
Copy link
Member

It's not entirely clear what this is about.

  • A mechanism to limit the number of concurrent connections accepted?
  • Implement ChannelTimeouts as available in OpenSSH since version 9.2: compare OpenSSH issue 3484?
  • Pointing out a problem, and if so, what exactly?
    • SftpClient not closed when ClientSession is closed?
    • Handle not closed when ClientSession or SftpClient closed?
    • Server doesn't close SftpSubsystems when ServerSession closes?
    • Server doesn't close ServerSession when ClientSession closes?

@beijishiqidu
Copy link
Author

@tomaswolf

Hello, friend.

I wrote all kinds of test cases, Here's just one example

private static SftpClient sftpClient;

    private static SftpFileSystem sftpFileSystem;

    public static void main(String[] args) throws Exception {

        Runnable runnable = new UploadFile();
        Thread thread1 = new Thread(runnable);
        thread1.start();
        thread1.join();

        Thread thread2 = new Thread(runnable);
        thread2.start();
        thread2.join();

        Thread thread3 = new Thread(runnable);
        thread3.start();
        thread3.join();
    }

    private static synchronized SftpClient getSftpClient() throws IOException {
        if (sftpClient != null) {
            return sftpClient;
        }
        sftpClient = getSftpFileSystem().getClient();
        return sftpClient;
    }

    private static synchronized SftpFileSystem getSftpFileSystem() throws IOException {
        if (sftpFileSystem != null) {
            return sftpFileSystem;
        }
        log.info("first init client");
        SshClient client = SshClient.setUpDefaultClient();
        client.start();

        ClientSession session = client.connect("admin", "127.0.0.1", 9120).verify().getSession();
        session.addPasswordIdentity("admin");
        session.auth().verify();
        session.setSessionHeartbeat(SessionHeartbeatController.HeartbeatType.IGNORE, Duration.ofMillis(5000));
        sftpFileSystem = DefaultSftpClientFactory.INSTANCE.createSftpFileSystem(session);
        return sftpFileSystem;
    }

    private static class UploadFile implements Runnable {

        @Override
        public void run() {
            for (int i = 0; i < 3; i++) {
                try (FileInputStream in = new FileInputStream(
                    "D:\\20230113\\202301131049" + i + ".dump")) {
                    SftpPath sftpPath = getSftpFileSystem().getPath(
                        "202301131049.dump" + Thread.currentThread().getId() + i + ".tmp");
                    Files.copy(in, sftpPath);
                    getSftpClient().rename("202301131049.dump" + Thread.currentThread().getId() + i + ".tmp",
                        "202301131049.dump" + Thread.currentThread().getId() + i + ".dump");
                } catch (Exception e) {
                    e.printStackTrace();
                }
            }
        }
    }

As in the example above, I just want to use the same SftpClient object in different threads.

However, I observe the server through jstack. Each time a thread is started, the server generates a thread of the SftpSubsystem object and is in the waiting state. But, in fact, my first thread's upload task is already complete.

# first time:
$ jstack.exe -l 56192 |grep sshd-Sftp
"sshd-SftpSubsystem-40668-thread-1" #41 daemon prio=5 os_prio=0 tid=0x00000000290e0000 nid=0xc67c runnable [0x0000000000b8e000]


# second time:
$ jstack.exe -l 56192 |grep sshd-Sftp
"sshd-SftpSubsystem-60480-thread-1" #42 daemon prio=5 os_prio=0 tid=0x00000000290e1800 nid=0x94c4 waiting on condition [0x000000002be5f000]
"sshd-SftpSubsystem-40668-thread-1" #41 daemon prio=5 os_prio=0 tid=0x00000000290e0000 nid=0xc67c waiting on condition [0x0000000000b8f000]

#third time:

$ jstack.exe -l 56192 |grep sshd-Sftp
"sshd-SftpSubsystem-57236-thread-1" #43 daemon prio=5 os_prio=0 tid=0x00000000290e4000 nid=0xa48c waiting on condition [0x000000002bf5f000]
"sshd-SftpSubsystem-60480-thread-1" #42 daemon prio=5 os_prio=0 tid=0x00000000290e1800 nid=0x94c4 waiting on condition [0x000000002be5f000]
"sshd-SftpSubsystem-40668-thread-1" #41 daemon prio=5 os_prio=0 tid=0x00000000290e0000 nid=0xc67c waiting on condition [0x0000000000b8f000]

and the detail jstack of thread sshd-SftpSubsystem-57236-thread-1 like this:

"sshd-SftpSubsystem-57236-thread-1" #31 daemon prio=5 os_prio=0 tid=0x000000002a046000 nid=0xe130 waiting on condition [0x000000000096f000]
   java.lang.Thread.State: WAITING (parking)
        at sun.misc.Unsafe.park(Native Method)
        - parking to wait for  <0x00000005c2b66680> (a java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject)
        at java.util.concurrent.locks.LockSupport.park(LockSupport.java:175)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(AbstractQueuedSynchronizer.java:2045)
        at java.util.concurrent.LinkedBlockingQueue.take(LinkedBlockingQueue.java:442)
        at org.apache.sshd.sftp.server.SftpSubsystem.run(SftpSubsystem.java:297)
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
        at java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:266)
        at java.util.concurrent.FutureTask.run(FutureTask.java)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)

   Locked ownable synchronizers:
        - <0x00000005c2b66898> (a java.util.concurrent.ThreadPoolExecutor$Worker)

I tried to look at the source code for this SftpSubsystem, and I found that it kept fetching values from the blocking queue.

try {
            ChannelSession channel = getServerChannelSession();
            LocalWindow localWindow = channel.getLocalWindow();
            while (true) {
                Buffer buffer = requests.take();
                if (buffer == CLOSE) {
                    break;
                }
                buffersCount++;
                process(buffer);
                localWindow.check();
            }
        } catch (Throwable t) {

So, I don't know how to close these objects, or if there's a timeout configuration that allows the object to be destroyed.

@tomaswolf
Copy link
Member

Maybe it would help if you closed the various client-side objects you use. An SftpClient is supposed to be closed when it is no longer needed.

But yes, there is a problem here. The SftpFileSystem uses a pool of SftpClients and uses separate clients per thread. This is a questionable architecture, especially since these SftpClients remain open (and I think, if used like in your last example, will even prevent them from being re-used). Each SftpClient is an SSH channel, which thus remains open, and thus the server-side SftpSubsystems don't exit.

A per-channel timeout on the server side could mitigate this (and other cases where clients don't properly close their SftpClients), but ultimately I think it is necessary to re-think the SftpFileSystem implementation such that it works without per-thread channels, and without ThreadLocals.

@tomaswolf tomaswolf added the bug An issue describing a bug in the code label May 19, 2023
@tomaswolf tomaswolf self-assigned this May 22, 2023
@tomaswolf tomaswolf added this to the 2.10.1 milestone May 22, 2023
tomaswolf added a commit to tomaswolf/mina-sshd that referenced this issue May 27, 2023
The previous implementation always put unused SftpClients back into
the pool, but SftpClients in the pool were never closed (unless the
whole SSH session was closed).

Let this pool work more like a Java thread pool: besides a maximum
size, give it a minimum "core" size, and a maximum life time for idle
channels in the pool, and remove them from the pool and close them
when they expire. By default, the maximum pool size is 8, the core size
1, and the idle life time 10 seconds.

Also drain the pool when the file system closes, and close all channels.

Remove the ThreadLocal. This mechanism was questionable anyway; it was
the source of multiple earlier bug reports and there are some scenarios
that it just cannot handle correctly. This change will mean that an
application using more threads on an SftpFileSystem instance than the
pool size may see poor SFTP performance on the extra threads. In this
case, the pool size should be increased, or the application redesigned.

Add some technical documentation on all this.

Ensure in SftpFileSystem.readDir(String) that the SftpClient on the
SftpIterableDirEntry is the wrapper. We don't want to close the
underlying pooled channel. Ditto for InputStream and OutputStream
returned from read or write: those also must use the wrapper to react
properly when the wrapper is closed.

Ensure the behavior of an SftpDirectoryStream is correct when the stream
is closed. According to the javadoc on DirectoryStream, the iterator may
continue to produce already cached entries, but then may exhaust early.

Bug: apache#371
tomaswolf added a commit to tomaswolf/mina-sshd that referenced this issue May 29, 2023
The previous implementation always put unused SftpClients back into
the pool, but SftpClients in the pool were never closed (unless the
whole SSH session was closed).

Let this pool work more like a Java thread pool: besides a maximum
size, give it a minimum "core" size, and a maximum life time for idle
channels in the pool, and remove them from the pool and close them
when they expire. By default, the maximum pool size is 8, the core size
1, and the idle life time 10 seconds.

Also drain the pool when the file system closes, and close all channels.

Remove the ThreadLocal. This mechanism was questionable anyway; it was
the source of multiple earlier bug reports and there are some scenarios
that it just cannot handle correctly. This change will mean that an
application using more threads on an SftpFileSystem instance than the
pool size may see poor SFTP performance on the extra threads. In this
case, the pool size should be increased, or the application redesigned.

Add some technical documentation on all this.

Ensure in SftpFileSystem.readDir(String) that the SftpClient on the
SftpIterableDirEntry is the wrapper. We don't want to close the
underlying pooled channel. Ditto for InputStream and OutputStream
returned from read or write: those also must use the wrapper to react
properly when the wrapper is closed.

Ensure the behavior of an SftpDirectoryStream is correct when the stream
is closed. According to the javadoc on DirectoryStream, the iterator may
continue to produce already cached entries, but then may exhaust early.

Bug: apache#371
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue describing a bug in the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants