This is an automated email from the ASF dual-hosted git repository.
lgoldstein pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
The following commit(s) were added to refs/heads/master by this push:
new 0072493 [SSHD-931] Using an executor supplier instead of a specific
instance in SftpSubsystemFactory
0072493 is described below
commit 0072493b2cd5a5972a73bad15902eb3698e443f4
Author: Lyor Goldstein <lgoldst...@apache.org>
AuthorDate: Mon Aug 5 09:41:51 2019 +0300
[SSHD-931] Using an executor supplier instead of a specific instance in
SftpSubsystemFactory
---
CHANGES.md | 14 +++++
docs/sftp.md | 70 ++++++++++++++--------
.../subsystem/sftp/SftpSubsystemFactory.java | 31 ++++++----
.../client/subsystem/sftp/SftpVersionsTest.java | 4 +-
.../helpers/SpaceAvailableExtensionImplTest.java | 2 +-
.../openssh/helpers/OpenSSHExtensionsTest.java | 2 +-
.../subsystem/sftp/SftpSubsystemFactoryTest.java | 17 +++---
7 files changed, 92 insertions(+), 48 deletions(-)
diff --git a/CHANGES.md b/CHANGES.md
index 5a416ae..f1b25ee 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -6,6 +6,18 @@
## Major code re-factoring
+* `SftpSubSystemFactory` and its `Builder` use a
`Supplier<CloseableExecutorService>` instead of
+an executor instance in order to allow users to provide a "fresh" instance
every time an SFTP
+session is initiated and protect their instance from shutdown when session is
destroyed:
+
+```java
+ CloseableExecutorService mySpecialExecutor = ...;
+ SftpSubsystemFactory factory = new SftpSubsystemFactory.Builder()
+ .withExecutorServiceProvider(() ->
ThreadUtils.noClose(mySpecialExecutor))
+ .build();
+ server.setSubsystemFactories(Collections.singletonList(factory));
+```
+
## Minor code helpers
* `SessionListener` supports `sessionPeerIdentificationReceived` that is
invoked once successful
@@ -16,5 +28,7 @@ peer version data is received.
* [SSHD-930](https://issues.apache.org/jira/browse/SSHD-930) - Added
configuration allowing the user to specify whether client should wait
for the server's identification before sending its own.
+* [SSHD-931](https://issues.apache.org/jira/browse/SSHD-931) - Using an
executor supplier instead of a specific instance in `SftpSubsystemFactory`
+
* [SSHD-934](https://issues.apache.org/jira/browse/SSHD-934) - Fixed ECDSA
public key encoding into OpenSSH format.
diff --git a/docs/sftp.md b/docs/sftp.md
index 49dee63..919d761 100644
--- a/docs/sftp.md
+++ b/docs/sftp.md
@@ -1,4 +1,4 @@
-## SFTP
+# SFTP
Both client-side and server-side SFTP are supported. Starting from version
2.0, the SFTP related code is located
in the `sshd-sftp` artifact, so one needs to add this additional dependency to
one's maven project:
@@ -13,18 +13,35 @@ in the `sshd-sftp` artifact, so one needs to add this
additional dependency to o
```
-### Server-side SFTP
+## Server-side SFTP
On the server side, the following code needs to be added:
```java
SftpSubsystemFactory factory = new SftpSubsystemFactory.Builder()
+ ...with...
+ ...with...
.build();
server.setSubsystemFactories(Collections.singletonList(factory));
```
+**Note:** the factory uses an ad-hoc `CloseableExecutorService` in order to
spawn the necessary threads
+for processing the protocol messages. The user can provide a custom `Supplier`
of such a service - however,
+it must be protected from shutdown if the user needs it to remain active
between successive SFTP session.
+This can be done via the `ThreadUtils#noClose` utility:
+
+```java
+
+ CloseableExecutorService mySpecialExecutor = ...;
+ SftpSubsystemFactory factory = new SftpSubsystemFactory.Builder()
+ .withExecutorServiceProvider(() ->
ThreadUtils.noClose(mySpecialExecutor))
+ .build();
+ server.setSubsystemFactories(Collections.singletonList(factory));
+
+```
+
### `SftpEventListener`
Provides information about major SFTP protocol events. The provided
`File/DirectoryHandle` to the various callbacks can also be used to
@@ -87,7 +104,29 @@ forces a synchronization of the data with the file-system.
This behavior can be
by setting the `sftp-auto-fsync-on-close` property to *false* (or by providing
a customized implementation
that involves other considerations as well).
-### Client-side SFTP
+### Internal exceptions and error message handling
+
+If an exception is thrown during processing of an SFTP command, then the
exception is translated into a `SSH_FXP_STATUS` message
+using a registered `SftpErrorStatusDataHandler`. The default implementation
provides a short description of the failure based on the thrown
+exception type. However, users may override it when creating the
`SftpSubsystemFactory` and provide their own codes and/or messages - e.g.,
+for debugging one can register a `DetailedSftpErrorStatusDataHandler` (see
`sshd-contrib`) that "leaks" more information in the generated message.
+
+If the registered handler implements `ChannelSessionAware` then it will also
be informed of the registered `ChannelSession` when it is
+provided to the `SftpSubsystem` itself. This can be used to register an
extended data writer that can handle data sent via the STDERR
+channel. **Note:** this feature is allowed according to [SFTP version 4 -
section
3.1](https://tools.ietf.org/html/draft-ietf-secsh-filexfer-04#section-3.1):
+
+>> Packets are sent and received on stdout and stdin. Data sent on stderr by
the server SHOULD be considered debug
+>> or supplemental error information, and MAY be displayed to the user.
+
+however, the current code provides no built-in support for this feature.
+
+If registering an extended data writer then one should take care of any race
conditions that may occur where (extended) data
+may arrive before the handler is informed of the existence of the
`ChannelSession`. For this purpose one should configure a
+reasonable buffer size by setting the `channel-session-max-extdata-bufsize`
property. This way, if any data arrives before the
+extended data handler is registered it will be buffered (up to the specified
max. size). **Note:** if a buffer size is configured
+but no extended data handler is registered when channel is spawning the
command then an exception will occur.
+
+## Client-side SFTP
In order to obtain an `SftpClient` instance one needs to use an
`SftpClientFactory`:
@@ -277,7 +316,7 @@ configuration keys and values.
```
-#### Configuring the client session used to create an `SftpFileSystem`
+### Configuring the client session used to create an `SftpFileSystem`
It is possible to register a `SftpFileSystemClientSessionInitializer` with the
provider instead of the default one
and thus better control the `ClientSession` used to generate the file-system
instance. The default implementation
@@ -363,6 +402,8 @@ UTF-8 is used. **Note:** the value can be a charset name or
a `java.nio.charset.
```
+## Extensions
+
Both client and server support several of the SFTP extensions specified in
various drafts:
* `supported` - [DRAFT 05 - section
4.4](http://tools.ietf.org/wg/secsh/draft-ietf-secsh-filexfer/draft-ietf-secsh-filexfer-05.tx)
@@ -458,24 +499,3 @@ for sending and receiving the newly added extension.
See how other extensions are implemented and follow their example
-### Internal exceptions and error message handling
-
-If an exception is thrown during processing of an SFTP command, then the
exception is translated into a `SSH_FXP_STATUS` message
-using a registered `SftpErrorStatusDataHandler`. The default implementation
provides a short description of the failure based on the thrown
-exception type. However, users may override it when creating the
`SftpSubsystemFactory` and provide their own codes and/or messages - e.g.,
-for debugging one can register a `DetailedSftpErrorStatusDataHandler` (see
`sshd-contrib`) that "leaks" more information in the generated message.
-
-If the registered handler implements `ChannelSessionAware` then it will also
be informed of the registered `ChannelSession` when it is
-provided to the `SftpSubsystem` itself. This can be used to register an
extended data writer that can handle data sent via the STDERR
-channel. **Note:** this feature is allowed according to [SFTP version 4 -
section
3.1](https://tools.ietf.org/html/draft-ietf-secsh-filexfer-04#section-3.1):
-
->> Packets are sent and received on stdout and stdin. Data sent on stderr by
the server SHOULD be considered debug
->> or supplemental error information, and MAY be displayed to the user.
-
-however, the current code provides no built-in support for this feature.
-
-If registering an extended data writer then one should take care of any race
conditions that may occur where (extended) data
-may arrive before the handler is informed of the existence of the
`ChannelSession`. For this purpose one should configure a
-reasonable buffer size by setting the `channel-session-max-extdata-bufsize`
property. This way, if any data arrives before the
-extended data handler is registered it will be buffered (up to the specified
max. size). **Note:** if a buffer size is configured
-but no extended data handler is registered when channel is spawning the
command then an exception will occur.
diff --git
a/sshd-sftp/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemFactory.java
b/sshd-sftp/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemFactory.java
index 056b5fa..0dfae8d 100644
---
a/sshd-sftp/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemFactory.java
+++
b/sshd-sftp/src/main/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemFactory.java
@@ -20,6 +20,7 @@
package org.apache.sshd.server.subsystem.sftp;
import java.util.Objects;
+import java.util.function.Supplier;
import org.apache.sshd.common.subsystem.sftp.SftpConstants;
import org.apache.sshd.common.util.GenericUtils;
@@ -39,7 +40,7 @@ public class SftpSubsystemFactory
public static final UnsupportedAttributePolicy DEFAULT_POLICY =
UnsupportedAttributePolicy.Warn;
public static class Builder extends AbstractSftpEventListenerManager
implements ObjectBuilder<SftpSubsystemFactory> {
- private CloseableExecutorService executors;
+ private Supplier<? extends CloseableExecutorService> executorsProvider;
private UnsupportedAttributePolicy policy = DEFAULT_POLICY;
private SftpFileSystemAccessor fileSystemAccessor =
SftpFileSystemAccessor.DEFAULT;
private SftpErrorStatusDataHandler errorStatusDataHandler =
SftpErrorStatusDataHandler.DEFAULT;
@@ -48,8 +49,8 @@ public class SftpSubsystemFactory
super();
}
- public Builder withExecutorService(CloseableExecutorService service) {
- executors = service;
+ public Builder withExecutorServiceProvider(Supplier<? extends
CloseableExecutorService> provider) {
+ executorsProvider = provider;
return this;
}
@@ -71,7 +72,7 @@ public class SftpSubsystemFactory
@Override
public SftpSubsystemFactory build() {
SftpSubsystemFactory factory = new SftpSubsystemFactory();
- factory.setExecutorService(executors);
+ factory.setExecutorServiceProvider(executorsProvider);
factory.setUnsupportedAttributePolicy(policy);
factory.setFileSystemAccessor(fileSystemAccessor);
factory.setErrorStatusDataHandler(errorStatusDataHandler);
@@ -80,7 +81,7 @@ public class SftpSubsystemFactory
}
}
- private CloseableExecutorService executors;
+ private Supplier<? extends CloseableExecutorService> executorsProvider;
private UnsupportedAttributePolicy policy = DEFAULT_POLICY;
private SftpFileSystemAccessor fileSystemAccessor =
SftpFileSystemAccessor.DEFAULT;
private SftpErrorStatusDataHandler errorStatusDataHandler =
SftpErrorStatusDataHandler.DEFAULT;
@@ -94,16 +95,17 @@ public class SftpSubsystemFactory
return NAME;
}
- public CloseableExecutorService getExecutorService() {
- return executors;
+ public Supplier<? extends CloseableExecutorService>
getExecutorServiceProvider() {
+ return executorsProvider;
}
/**
- * @param service The {@link CloseableExecutorService} to be used by the
{@link SftpSubsystem}
- * command when starting execution. If {@code null} then a single-threaded
ad-hoc service is used.
+ * @param service The {@link Supplier} of {@link
CloseableExecutorService}-s to be used by the
+ * {@link SftpSubsystem} command when starting execution. If {@code null}
then a single-threaded
+ * ad-hoc service is used.
*/
- public void setExecutorService(CloseableExecutorService service) {
- executors = service;
+ public void setExecutorServiceProvider(Supplier<? extends
CloseableExecutorService> provider) {
+ executorsProvider = provider;
}
public UnsupportedAttributePolicy getUnsupportedAttributePolicy() {
@@ -136,10 +138,15 @@ public class SftpSubsystemFactory
errorStatusDataHandler = Objects.requireNonNull(handler, "No error
status data handler provided");
}
+ protected CloseableExecutorService resolveExecutorService() {
+ Supplier<? extends CloseableExecutorService> provider =
getExecutorServiceProvider();
+ return (provider == null) ? null : provider.get();
+ }
+
@Override
public Command create() {
SftpSubsystem subsystem =
- new SftpSubsystem(getExecutorService(),
+ new SftpSubsystem(resolveExecutorService(),
getUnsupportedAttributePolicy(), getFileSystemAccessor(),
getErrorStatusDataHandler());
GenericUtils.forEach(getRegisteredListeners(),
subsystem::addSftpEventListener);
diff --git
a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpVersionsTest.java
b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpVersionsTest.java
index 80d149f..af3514a 100644
---
a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpVersionsTest.java
+++
b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpVersionsTest.java
@@ -245,7 +245,7 @@ public class SftpVersionsTest extends
AbstractSftpClientTestSupport {
SftpSubsystemFactory factory = new SftpSubsystemFactory() {
@Override
public Command create() {
- SftpSubsystem subsystem = new
SftpSubsystem(getExecutorService(),
+ SftpSubsystem subsystem = new
SftpSubsystem(resolveExecutorService(),
getUnsupportedAttributePolicy(),
getFileSystemAccessor(), getErrorStatusDataHandler()) {
@Override
protected NavigableMap<String, Object>
resolveFileAttributes(Path file, int flags, LinkOption... options)
@@ -369,7 +369,7 @@ public class SftpVersionsTest extends
AbstractSftpClientTestSupport {
SftpSubsystemFactory factory = new SftpSubsystemFactory() {
@Override
public Command create() {
- SftpSubsystem subsystem = new
SftpSubsystem(getExecutorService(),
+ SftpSubsystem subsystem = new
SftpSubsystem(resolveExecutorService(),
getUnsupportedAttributePolicy(),
getFileSystemAccessor(), getErrorStatusDataHandler()) {
@Override
protected NavigableMap<String, Object>
resolveFileAttributes(Path file, int flags, LinkOption... options)
diff --git
a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/extensions/helpers/SpaceAvailableExtensionImplTest.java
b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/extensions/helpers/SpaceAvailableExtensionImplTest.java
index a528278..a2a73e1 100644
---
a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/extensions/helpers/SpaceAvailableExtensionImplTest.java
+++
b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/extensions/helpers/SpaceAvailableExtensionImplTest.java
@@ -71,7 +71,7 @@ public class SpaceAvailableExtensionImplTest extends
AbstractSftpClientTestSuppo
sshd.setSubsystemFactories(Collections.singletonList(new
SftpSubsystemFactory() {
@Override
public Command create() {
- return new SftpSubsystem(getExecutorService(),
+ return new SftpSubsystem(resolveExecutorService(),
getUnsupportedAttributePolicy(),
getFileSystemAccessor(), getErrorStatusDataHandler()) {
@Override
protected SpaceAvailableExtensionInfo doSpaceAvailable(int
id, String path) throws IOException {
diff --git
a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/extensions/openssh/helpers/OpenSSHExtensionsTest.java
b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/extensions/openssh/helpers/OpenSSHExtensionsTest.java
index 5e31a23..4cc7a46 100644
---
a/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/extensions/openssh/helpers/OpenSSHExtensionsTest.java
+++
b/sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/extensions/openssh/helpers/OpenSSHExtensionsTest.java
@@ -126,7 +126,7 @@ public class OpenSSHExtensionsTest extends
AbstractSftpClientTestSupport {
sshd.setSubsystemFactories(Collections.singletonList(new
SftpSubsystemFactory() {
@Override
public Command create() {
- return new SftpSubsystem(getExecutorService(),
+ return new SftpSubsystem(resolveExecutorService(),
getUnsupportedAttributePolicy(),
getFileSystemAccessor(), getErrorStatusDataHandler()) {
@Override
protected List<OpenSSHExtension>
resolveOpenSSHExtensions(ServerSession session) {
diff --git
a/sshd-sftp/src/test/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemFactoryTest.java
b/sshd-sftp/src/test/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemFactoryTest.java
index da722f2..45799b2 100644
---
a/sshd-sftp/src/test/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemFactoryTest.java
+++
b/sshd-sftp/src/test/java/org/apache/sshd/server/subsystem/sftp/SftpSubsystemFactoryTest.java
@@ -45,7 +45,7 @@ public class SftpSubsystemFactoryTest extends
JUnitTestSupport {
@Test
public void testBuilderDefaultFactoryValues() {
SftpSubsystemFactory factory = new
SftpSubsystemFactory.Builder().build();
- assertNull("Mismatched executor", factory.getExecutorService());
+ assertNull("Mismatched executor", factory.resolveExecutorService());
assertSame("Mismatched unsupported attribute policy",
SftpSubsystemFactory.DEFAULT_POLICY, factory.getUnsupportedAttributePolicy());
}
@@ -56,9 +56,10 @@ public class SftpSubsystemFactoryTest extends
JUnitTestSupport {
public void testBuilderCorrectlyInitializesFactory() {
SftpSubsystemFactory.Builder builder = new
SftpSubsystemFactory.Builder();
CloseableExecutorService service = dummyExecutor();
- SftpSubsystemFactory factory = builder.withExecutorService(service)
+ SftpSubsystemFactory factory =
+ builder.withExecutorServiceProvider(() -> service)
.build();
- assertSame("Mismatched executor", service,
factory.getExecutorService());
+ assertSame("Mismatched executor", service,
factory.resolveExecutorService());
for (UnsupportedAttributePolicy policy :
UnsupportedAttributePolicy.VALUES) {
SftpSubsystemFactory actual =
builder.withUnsupportedAttributePolicy(policy).build();
@@ -82,13 +83,15 @@ public class SftpSubsystemFactoryTest extends
JUnitTestSupport {
@Test
public void testBuilderUniqueInstance() {
SftpSubsystemFactory.Builder builder = new
SftpSubsystemFactory.Builder();
- SftpSubsystemFactory f1 =
builder.withExecutorService(dummyExecutor()).build();
+ CloseableExecutorService service1 = dummyExecutor();
+ SftpSubsystemFactory f1 = builder.withExecutorServiceProvider(() ->
service1).build();
SftpSubsystemFactory f2 = builder.build();
assertNotSame("No new instance built", f1, f2);
- assertSame("Mismatched executors", f1.getExecutorService(),
f2.getExecutorService());
+ assertSame("Mismatched executors", f1.resolveExecutorService(),
f2.resolveExecutorService());
- SftpSubsystemFactory f3 =
builder.withExecutorService(dummyExecutor()).build();
- assertNotSame("Executor service not changed", f1.getExecutorService(),
f3.getExecutorService());
+ CloseableExecutorService service2 = dummyExecutor();
+ SftpSubsystemFactory f3 = builder.withExecutorServiceProvider(() ->
service2).build();
+ assertNotSame("Executor service not changed",
f1.resolveExecutorService(), f3.resolveExecutorService());
}
private static CloseableExecutorService dummyExecutor() {
