Skip to content

Commit 31e73f7

Browse files
committed
apacheGH-371: Re-implement the channel pool of an SftpFileSystem
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
1 parent f1994bd commit 31e73f7

File tree

10 files changed

+762
-73
lines changed

10 files changed

+762
-73
lines changed

CHANGES.md

+20
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,27 @@
2727
## Bug Fixes
2828

2929
* [GH-370](https://github.com/apache/mina-sshd/issues/370) Also compare file keys in `ModifiableFileWatcher`.
30+
* [GH-371](https://github.com/apache/mina-sshd/issues/371) Fix channel pool in `SftpFileSystem`.
3031

3132

3233
* [SSHD-1310](https://issues.apache.org/jira/browse/SSHD-1310) `SftpFileSystem`: do not close user session.
3334
* [SSHD-1327](https://issues.apache.org/jira/browse/SSHD-1327) `ChannelAsyncOutputStream`: remove write future when done.
35+
36+
## New Features
37+
38+
* [GH-356](https://github.com/apache/mina-sshd/issues/356) Publish snapshot maven artifacts to the [Apache Snapshots](https://repository.apache.org/content/repositories/snapshots) maven repository.
39+
40+
41+
## Major Code Re-factoring
42+
43+
As part of the fix for [GH-371](https://github.com/apache/mina-sshd/issues/371)
44+
the channel pool in `SftpFileSystem` was rewritten completely. Previous code also
45+
used `ThreadLocal`s to store `SftpClient`s, which could cause memory leaks.
46+
47+
These `ThreadLocal`s have been removed, and the channel pool has been rewritten
48+
to function similar to a Java `ThreadPool`: the pool has a maximum size; it has
49+
an expiration duration after which an idle channel is removed and closed, and
50+
it has a "core size" of channels to keep even if they are idle. If a channel is
51+
closed for any reason it is evicted from the pool.
52+
53+
Properties to configure these pool parameters have been added to `SftpModuleProperties`.

docs/sftp.md

+2
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,8 @@ does not allow colon (`:`) in either one in order to avoid parsing confusion. Se
307307
>> deprecated ... Applications may choose to ignore or reject such
308308
>> data when it is received as part of a reference...
309309
310+
See also the technical notes on the [SftpFileSystem](./technical/sft_filesystem.md).
311+
310312
#### Configuring the `SftpFileSystemProvider`
311313

312314
When "mounting" a new file system one can provide extra configuration parameters using either the

docs/technical/sftp_filesystem.md

+124
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
# The `SftpFileSystem`
2+
3+
Class `SftpFileSystem` is an implementation of `java.nio.file.FileSystem` and
4+
lets client code treat an SFTP server like any other file system. It is a
5+
*remote* file system, though, and that has some effects that clients have
6+
to aware of. Because operations are *remote* operations involving network
7+
requests and answers, the performance characteristics are different than
8+
most other file systems.
9+
10+
# Creating an `SftpFileSystem`
11+
12+
An `SftpFileSystem` needs an SSH session to be able to talk to the SFTP server.
13+
14+
There are two ways to create an `SftpFileSystem`:
15+
16+
1. If you already have an SSH `ClientSession`, you can create the file system
17+
off that session using `SftpClientFactory.instance().createSftpFileSystem()`.
18+
The file system remains valid until it is closed, or until the session is
19+
closed. When the file system is closed, the session will *not* be closed.
20+
21+
2. You can create an `SftpFileSystem` with a `sftp://` URI using the standard
22+
Java factory `java.nio.file.FileSystems.newFileSystem()`. This will automatically
23+
create an `SshClient` with default settings, and the file system will open
24+
an SSH session itself. This session has heartbeats enabled to keep it open
25+
for as long as the file system is open. The file system remains valid until
26+
closed, at which point it will close the session it had created.
27+
28+
In either case, the file system will be closed if the session closes.
29+
30+
# SSH Resource Management
31+
32+
Most operations on an `SftpFileSystem`, or on streams returned, produce SFTP
33+
requests over the network, and wait for a reply to be received. This works
34+
internally by using `SftpClient` to talk to the SFTP server. An `SftpClient`
35+
is the client-side implementation of the SFTP protocol over an SSH channel
36+
(a `ChannelSession`).
37+
38+
The SSH channel and the `SftpClient` are tightly coupled: the channel is
39+
opened when the `SftpClient`is initialized, and the channel is closed when
40+
the `SftpClient` is closed, and when the channel closes, so is the `SftpClient`.
41+
42+
For `SftpFileSystem` it would be rather inefficient to use a new `SftpClient`
43+
for each new operation. That would create a new channel every time, and tear
44+
it down after the operation. But channels have a setup cost, and the SFTP
45+
protocol also has to initialized, both of which involve exchanging messages
46+
over the network. This is not efficient if one wants to perform multiple
47+
operations, such as transferring multiple files with `java.nio.file.Files.copy()`.
48+
49+
## The Channel Pool
50+
51+
The `SftpFileSystem` thus employs a *pool* of `SftpClient`s. This pool is
52+
initially empty. The first operation will create an `SftpClient`, initialize
53+
it, and then perform its operation. But then it will add the still open
54+
`SftpClient` to the pool for use by subsequent operations instead of closing
55+
it. The next operation can then simply grab this already initialized `SftpClient`
56+
with its open channel and perform its operation.
57+
58+
The pool is limited by a maximum size of `SftpModuleProperties.POOL_SIZE` (by
59+
default 8). The pool can grow to this size if there are that many threads that
60+
perform operations on the `SftpFileSystem` concurrently.
61+
62+
`SftpClient`s in the pool need to be closed at some point. Consider an application
63+
that has a burst of file transfers and uses 8 threads to perform them. Afterwards,
64+
the pool will contain 8 `SftpClient`s: that's 8 open SSH channels, each with an SFTP
65+
subsystem at the server's end. If the application then does only little, like
66+
transferring a few files sequentially over the next few hours, until the next burst
67+
(which may never come), then we don't want to keep all 8 channels open and consuming
68+
resources not only in the client but also on the server side.
69+
70+
(This assumes that the whole SSH session remains open for that long, which can be
71+
accomplished by using heartbeats on the session.)
72+
73+
The `SftpFileSystem` handles this by expiring inactive clients from the pool. If a
74+
client has been in the pool for `SftpModuleProperties.POOL_LIFE_TIME` (default is 10
75+
seconds), it is removed from the pool and closed. (If it was in the pool for that
76+
time, this means it was idle for that time: no operation was performed on it.) If
77+
no operation on the `SftpFileSystem` occurs at all for this time, it's possible that
78+
the pool is emptied, and the next operation has to create and initialize a new
79+
`SftpClient`and channel.
80+
81+
If an application doesn't want this, it can define `SftpModuleProperties.POOL_CORE_SIZE`,
82+
which must be smaller than `POOL_SIZE`. By default, it is zero. If greater than zero,
83+
that many `SftpClient`s are kept in the pool (and that many channels are kept open)
84+
even if they are idle.
85+
86+
It should be noted that the SFTP server may also decide to close channels whenever
87+
it wants. This will close the channel and the `SftpClient`on the client side. If it
88+
happens while a client-side operation is ongoing, the operation will fail with an
89+
exception; it it happens on an idle `SftpClient` in the pool, the `SftpClient` is
90+
simply removed from the pool.
91+
92+
If the whole SSH session is closed, the `SftpFileSystem` is closed. When a
93+
`SftpFileSystem` is closed, all `SftpClient`s in the pool are closed, and no new
94+
clients will be added to the pool.
95+
96+
## Choosing the Pool Size
97+
98+
If there are more then `POOL_SIZE` threads using the same `SftpFileSystem`, it is
99+
possible that all `POOL_SIZE` clients are already in use when a thread tries to
100+
do a file system operation. In this case, a new `SftpClient` is created, which
101+
will be closed after the operation. This is a sign of the `POOL_SIZE` being too
102+
small for the application, or that the application is badly designed. Using too
103+
many threads for remote file operations is not a good idea in SFTP: all traffic
104+
in the end goes over a single network connection anyway. Using a limited number
105+
of threads may bring some speedup compared to strictly sequential operations
106+
because the handling of the data received is offloaded to these threads, while
107+
the next message can already be sent or received. But copying 1000 files using
108+
1000 threads and SSH channels is nonsense; it's far better to handle that many
109+
files in batches with a smaller number of threads (maybe 8).
110+
111+
In any case, `SftpModuleProperties.POOL_SIZE` should be large enough to accommodate
112+
the number of threads the client application is going to use for operations on
113+
the `SftpFileSystem`. If there are more threads, performance may degrade.
114+
115+
Versions of Apache MINA sshd <= 2.10.0 tried to mitigate this performance drop
116+
for such "extra" threads by keeping the `SftpClient` in a `ThreadLocal`, so that
117+
such "extra" threads could re-use the `SftpClient`. This mechanism has been *removed*
118+
because it sometimes caused memory leaks. The mechanism was also flawed because
119+
there were use cases where it just could not work correctly.
120+
121+
Design your application such that is uses a small maximum number of threads that
122+
perform operations on a `SftpFileSystem`instance. Set `SftpModuleProperties.POOL_SIZE`
123+
such that it is >= the maximum number of threads that operate concurrently on the
124+
file system. The default pool size is 8.

sshd-sftp/src/main/java/org/apache/sshd/sftp/SftpModuleProperties.java

+28-1
Original file line numberDiff line numberDiff line change
@@ -60,11 +60,38 @@ public final class SftpModuleProperties {
6060
= Property.duration("sftp-channel-open-timeout", Duration.ofSeconds(15L));
6161

6262
/**
63-
* See {@link org.apache.sshd.sftp.client.fs.SftpFileSystem}.
63+
* The maximum size of the channel pool used by an {@link org.apache.sshd.sftp.client.fs.SftpFileSystem}; by default
64+
* 8. The value must be &gt; zero.
65+
*
66+
* @see org.apache.sshd.sftp.client.fs.SftpFileSystem
6467
*/
6568
public static final Property<Integer> POOL_SIZE
6669
= Property.integer("sftp-fs-pool-size", 8);
6770

71+
/**
72+
* A timeout after which idle channels in the pool of an {@link org.apache.sshd.sftp.client.fs.SftpFileSystem} are
73+
* removed from the pool and closed; by default 10 seconds. If set to zero, channels in the pool will not expire and
74+
* will be closed only then the file system is closed, or if the server closes them.
75+
* <p>
76+
* The duration should not be shorter than 1 millisecond. If it is, 1 millisecond will be assumed.
77+
* </p>
78+
*
79+
* @see org.apache.sshd.sftp.client.fs.SftpFileSystem
80+
*/
81+
public static final Property<Duration> POOL_LIFE_TIME
82+
= Property.duration("sftp-fs-pool-life-time", Duration.ofSeconds(10));
83+
84+
/**
85+
* If &gt;= 0, that many channels may be kept open in the channel pool of an
86+
* {@link org.apache.sshd.sftp.client.fs.SftpFileSystem} even if they are idle; by default 1. If &gt;=
87+
* {@link #POOL_SIZE}, channels will not expire and will be closed only then the file system is closed, or if the
88+
* server closes them.
89+
*
90+
* @see org.apache.sshd.sftp.client.fs.SftpFileSystem
91+
*/
92+
public static final Property<Integer> POOL_CORE_SIZE
93+
= Property.integer("sftp-fs-pool-core-size", 1);
94+
6895
/**
6996
* See {@link org.apache.sshd.sftp.client.fs.SftpFileSystemProvider}.
7097
*/

sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpDirectoryStream.java

+7-4
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
*/
3535
public class SftpDirectoryStream implements SftpClientHolder, DirectoryStream<Path> {
3636
protected SftpPathIterator pathIterator;
37+
protected boolean pathIteratorConsumed;
3738

3839
private final SftpPath path;
3940
private final Filter<? super Path> filter;
@@ -93,17 +94,19 @@ public Iterator<Path> iterator() {
9394
/*
9495
* According to documentation this method can be called only once
9596
*/
96-
if (pathIterator == null) {
97+
if (pathIteratorConsumed) {
9798
throw new IllegalStateException("Iterator has already been consumed");
9899
}
99100

100-
Iterator<Path> iter = pathIterator;
101-
pathIterator = null;
102-
return iter;
101+
pathIteratorConsumed = true;
102+
return pathIterator;
103103
}
104104

105105
@Override
106106
public void close() throws IOException {
107+
if (pathIterator != null) {
108+
pathIterator.close();
109+
}
107110
sftp.close();
108111
}
109112
}

0 commit comments

Comments
 (0)