GH-9272: Close ClientSession from SftpSession

Fixes: #9272

* close ClientSession when closing SftpSession
* fix whitespace issues
* stop the SshClient on bean destruction
* use convenient assertions

(cherry picked from commit a3fb68a831)
This commit is contained in:
darrylsmithUGA
2024-07-01 13:02:35 -04:00
committed by Spring Builds
parent 46c8589a49
commit 587bad90be
10 changed files with 137 additions and 28 deletions

View File

@@ -1,5 +1,5 @@
/*
* Copyright 2002-2023 the original author or authors.
* Copyright 2002-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -49,6 +49,7 @@ import org.apache.sshd.sftp.client.SftpVersionSelector;
import org.apache.sshd.sftp.client.impl.AbstractSftpClient;
import org.apache.sshd.sftp.client.impl.DefaultSftpClient;
import org.springframework.beans.factory.DisposableBean;
import org.springframework.core.io.Resource;
import org.springframework.integration.context.IntegrationContextUtils;
import org.springframework.integration.file.remote.session.SessionFactory;
@@ -74,10 +75,11 @@ import org.springframework.util.Assert;
* @author Auke Zaaiman
* @author Christian Tzolov
* @author Adama Sorho
* @author Darryl Smith
*
* @since 2.0
*/
public class DefaultSftpSessionFactory implements SessionFactory<SftpClient.DirEntry>, SharedSessionCapable {
public class DefaultSftpSessionFactory implements SessionFactory<SftpClient.DirEntry>, SharedSessionCapable, DisposableBean {
private final Lock lock = new ReentrantLock();
@@ -421,6 +423,13 @@ public class DefaultSftpSessionFactory implements SessionFactory<SftpClient.DirE
return new ConcurrentSftpClient(clientSession, initialVersionSelector, errorDataHandler);
}
@Override
public void destroy() throws Exception {
if (this.isInnerClient && this.sshClient != null && this.sshClient.isStarted()) {
this.sshClient.stop();
}
}
/**
* The {@link DefaultSftpClient} extension to lock the {@link #send(int, Buffer)}
* for concurrent interaction.

View File

@@ -25,6 +25,7 @@ import java.time.Duration;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
import org.apache.sshd.client.session.ClientSession;
import org.apache.sshd.common.util.net.SshdSocketAddress;
import org.apache.sshd.sftp.SftpModuleProperties;
import org.apache.sshd.sftp.client.SftpClient;
@@ -47,6 +48,7 @@ import org.springframework.util.StringUtils;
* @author Gary Russell
* @author Artem Bilan
* @author Christian Tzolov
* @author Darryl Smith
* @since 2.0
*/
public class SftpSession implements Session<SftpClient.DirEntry> {
@@ -156,6 +158,16 @@ public class SftpSession implements Session<SftpClient.DirEntry> {
catch (IOException ex) {
throw new UncheckedIOException("failed to close an SFTP client", ex);
}
try {
ClientSession session = this.sftpClient.getSession();
if (session != null && session.isOpen()) {
session.close();
}
}
catch (IOException ex) {
throw new UncheckedIOException("failed to close an SFTP client (session)", ex);
}
}
@Override

View File

@@ -31,6 +31,7 @@ import org.junit.jupiter.api.condition.DisabledOnOs;
import org.junit.jupiter.api.condition.OS;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.integration.IntegrationMessageHeaderAccessor;
import org.springframework.integration.channel.QueueChannel;
@@ -43,6 +44,7 @@ import org.springframework.integration.dsl.context.IntegrationFlowContext.Integr
import org.springframework.integration.file.FileHeaders;
import org.springframework.integration.file.remote.RemoteFileTemplate;
import org.springframework.integration.file.remote.gateway.AbstractRemoteFileOutboundGateway;
import org.springframework.integration.file.remote.session.SessionFactory;
import org.springframework.integration.file.support.FileExistsMode;
import org.springframework.integration.sftp.SftpTestSupport;
import org.springframework.integration.sftp.session.SftpRemoteFileTemplate;
@@ -59,6 +61,7 @@ import static org.assertj.core.api.Assertions.assertThat;
* @author Gary Russell
* @author Joaquin Santana
* @author Deepak Gunasekaran
* @author Darryl Smith
*
* @since 5.0
*/
@@ -69,11 +72,14 @@ public class SftpTests extends SftpTestSupport {
@Autowired
private IntegrationFlowContext flowContext;
@Autowired
private SessionFactory<SftpClient.DirEntry> sessionFactory;
@Test
public void testSftpInboundFlow() {
QueueChannel out = new QueueChannel();
IntegrationFlow flow = IntegrationFlow
.from(Sftp.inboundAdapter(sessionFactory())
.from(Sftp.inboundAdapter(sessionFactory)
.preserveTimestamp(true)
.remoteDirectory("/sftpSource")
.regexFilter(".*\\.txt$")
@@ -106,7 +112,7 @@ public class SftpTests extends SftpTestSupport {
public void testSftpInboundStreamFlow() throws Exception {
QueueChannel out = new QueueChannel();
StandardIntegrationFlow flow = IntegrationFlow.from(
Sftp.inboundStreamingAdapter(new SftpRemoteFileTemplate(sessionFactory()))
Sftp.inboundStreamingAdapter(new SftpRemoteFileTemplate(sessionFactory))
.remoteDirectory("sftpSource")
.regexFilter(".*\\.txt$"),
e -> e.id("sftpInboundAdapter").poller(Pollers.fixedDelay(100)))
@@ -133,7 +139,7 @@ public class SftpTests extends SftpTestSupport {
@Test
public void testSftpOutboundFlow() {
IntegrationFlow flow = f -> f.handle(Sftp.outboundAdapter(sessionFactory(), FileExistsMode.FAIL)
IntegrationFlow flow = f -> f.handle(Sftp.outboundAdapter(sessionFactory, FileExistsMode.FAIL)
.useTemporaryFileName(false)
.fileNameExpression("headers['" + FileHeaders.FILENAME + "']")
.remoteDirectory("sftpTarget"));
@@ -143,7 +149,7 @@ public class SftpTests extends SftpTestSupport {
.setHeader(FileHeaders.FILENAME, fileName)
.build());
RemoteFileTemplate<SftpClient.DirEntry> template = new RemoteFileTemplate<>(sessionFactory());
RemoteFileTemplate<SftpClient.DirEntry> template = new RemoteFileTemplate<>(sessionFactory);
SftpClient.DirEntry[] files =
template.execute(session -> session.list(getTargetRemoteDirectory().getName() + "/" + fileName));
assertThat(files.length).isEqualTo(1);
@@ -154,7 +160,7 @@ public class SftpTests extends SftpTestSupport {
@Test
public void testSftpOutboundFlowSftpTemplate() {
SftpRemoteFileTemplate sftpTemplate = new SftpRemoteFileTemplate(sessionFactory());
SftpRemoteFileTemplate sftpTemplate = new SftpRemoteFileTemplate(sessionFactory);
IntegrationFlow flow = f -> f.handle(Sftp.outboundAdapter(sftpTemplate)
.useTemporaryFileName(false)
.fileNameExpression("headers['" + FileHeaders.FILENAME + "']")
@@ -175,7 +181,7 @@ public class SftpTests extends SftpTestSupport {
@Test
public void testSftpOutboundFlowSftpTemplateAndMode() {
SftpRemoteFileTemplate sftpTemplate = new SftpRemoteFileTemplate(sessionFactory());
SftpRemoteFileTemplate sftpTemplate = new SftpRemoteFileTemplate(sessionFactory);
IntegrationFlow flow = f -> f.handle(Sftp.outboundAdapter(sftpTemplate, FileExistsMode.APPEND)
.useTemporaryFileName(false)
.fileNameExpression("headers['" + FileHeaders.FILENAME + "']")
@@ -200,7 +206,7 @@ public class SftpTests extends SftpTestSupport {
@Test
@DisabledOnOs(OS.WINDOWS)
public void testSftpOutboundFlowWithChmod() {
IntegrationFlow flow = f -> f.handle(Sftp.outboundAdapter(sessionFactory(), FileExistsMode.FAIL)
IntegrationFlow flow = f -> f.handle(Sftp.outboundAdapter(sessionFactory, FileExistsMode.FAIL)
.useTemporaryFileName(false)
.fileNameExpression("headers['" + FileHeaders.FILENAME + "']")
.chmod(0644)
@@ -211,7 +217,7 @@ public class SftpTests extends SftpTestSupport {
.setHeader(FileHeaders.FILENAME, fileName)
.build());
RemoteFileTemplate<SftpClient.DirEntry> template = new RemoteFileTemplate<>(sessionFactory());
RemoteFileTemplate<SftpClient.DirEntry> template = new RemoteFileTemplate<>(sessionFactory);
SftpClient.DirEntry[] files =
template.execute(session -> session.list(getTargetRemoteDirectory().getName() + "/" + fileName));
assertThat(files.length).isEqualTo(1);
@@ -230,7 +236,7 @@ public class SftpTests extends SftpTestSupport {
public void testSftpMgetFlow() {
QueueChannel out = new QueueChannel();
IntegrationFlow flow = f -> f
.handle(Sftp.outboundGateway(sessionFactory(), AbstractRemoteFileOutboundGateway.Command.MGET,
.handle(Sftp.outboundGateway(sessionFactory, AbstractRemoteFileOutboundGateway.Command.MGET,
"payload")
.options(AbstractRemoteFileOutboundGateway.Option.RECURSIVE)
.regexFileNameFilter("(subSftpSource|.*1.txt)")
@@ -259,7 +265,7 @@ public class SftpTests extends SftpTestSupport {
public void testSftpSessionCallback() {
QueueChannel out = new QueueChannel();
IntegrationFlow flow = f -> f
.<String>handle((p, h) -> new SftpRemoteFileTemplate(sessionFactory()).execute(s -> s.list(p)))
.<String>handle((p, h) -> new SftpRemoteFileTemplate(sessionFactory).execute(s -> s.list(p)))
.channel(out);
IntegrationFlowRegistration registration = this.flowContext.registration(flow).register();
registration.getInputChannel().send(new GenericMessage<>("sftpSource"));
@@ -277,7 +283,7 @@ public class SftpTests extends SftpTestSupport {
public void testSftpMv() {
QueueChannel out = new QueueChannel();
IntegrationFlow flow = f -> f
.handle(Sftp.outboundGateway(sessionFactory(), AbstractRemoteFileOutboundGateway.Command.MV, "payload")
.handle(Sftp.outboundGateway(sessionFactory, AbstractRemoteFileOutboundGateway.Command.MV, "payload")
.renameExpression("payload.concat('.done')")
.remoteDirectoryExpression("'sftpSource'"))
.channel(out);
@@ -301,6 +307,11 @@ public class SftpTests extends SftpTestSupport {
@EnableIntegration
public static class ContextConfiguration {
@Bean
public SessionFactory<SftpClient.DirEntry> ftpsessionFactory() {
return SftpTests.sessionFactory();
}
}
}

View File

@@ -61,6 +61,7 @@ import static org.mockito.Mockito.when;
* @author Gary Russell
* @author Artem Bilan
* @author Joaquin Santana
* @author Darryl Smith
*
* @since 2.0
*/
@@ -160,6 +161,8 @@ public class SftpInboundRemoteFileSystemSynchronizerTests {
ms.stop();
verify(synchronizer).close();
verify(store).close();
ftpSessionFactory.destroy();
}
public static class TestSftpSessionFactory extends DefaultSftpSessionFactory {

View File

@@ -1,5 +1,5 @@
/*
* Copyright 2018-2022 the original author or authors.
* Copyright 2018-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -16,12 +16,15 @@
package org.springframework.integration.sftp.inbound;
import org.apache.sshd.sftp.client.SftpClient;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.ApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.integration.file.FileHeaders;
import org.springframework.integration.file.remote.session.SessionFactory;
import org.springframework.integration.sftp.SftpTestSupport;
import org.springframework.messaging.Message;
import org.springframework.test.annotation.DirtiesContext;
@@ -32,6 +35,7 @@ import static org.assertj.core.api.Assertions.assertThat;
/**
* @author Gary Russell
* @author Artem bilan
* @author Darryl Smith
*
* @since 5.0.7
*
@@ -43,6 +47,9 @@ public class SftpMessageSourceTests extends SftpTestSupport {
@Autowired
private ApplicationContext context;
@Autowired
private SessionFactory<SftpClient.DirEntry> sessionFactory;
@Test
public void testMaxFetch() {
SftpInboundFileSynchronizingMessageSource messageSource = buildSource();
@@ -53,7 +60,7 @@ public class SftpMessageSourceTests extends SftpTestSupport {
}
private SftpInboundFileSynchronizingMessageSource buildSource() {
SftpInboundFileSynchronizer sync = new SftpInboundFileSynchronizer(sessionFactory());
SftpInboundFileSynchronizer sync = new SftpInboundFileSynchronizer(sessionFactory);
sync.setRemoteDirectory("/sftpSource/");
sync.setBeanFactory(this.context);
SftpInboundFileSynchronizingMessageSource messageSource = new SftpInboundFileSynchronizingMessageSource(sync);
@@ -68,6 +75,11 @@ public class SftpMessageSourceTests extends SftpTestSupport {
@Configuration
public static class Config {
@Bean
public SessionFactory<SftpClient.DirEntry> ftpSessionFactory() {
return SftpMessageSourceTests.sessionFactory();
}
}
}

View File

@@ -76,6 +76,7 @@ import static org.mockito.Mockito.when;
* @author Gary Russell
* @author Gunnar Hillert
* @author Artem Bilan
* @author Darryl Smith
*/
public class SftpOutboundTests {
@@ -84,7 +85,7 @@ public class SftpOutboundTests {
File targetDir = new File("remote-target-dir");
assertThat(targetDir.exists()).as("target directory does not exist: " + targetDir.getName()).isTrue();
SessionFactory<SftpClient.DirEntry> sessionFactory = new TestSftpSessionFactory();
TestSftpSessionFactory sessionFactory = new TestSftpSessionFactory();
FileTransferringMessageHandler<SftpClient.DirEntry> handler =
new FileTransferringMessageHandler<>(sessionFactory);
handler.setRemoteDirectoryExpression(new LiteralExpression(targetDir.getName()));
@@ -103,6 +104,8 @@ public class SftpOutboundTests {
handler.handleMessage(new GenericMessage<>(srcFile));
assertThat(destFile.exists()).as("destination file was not created").isTrue();
sessionFactory.destroy();
}
@Test
@@ -111,7 +114,7 @@ public class SftpOutboundTests {
if (file.exists()) {
file.delete();
}
SessionFactory<SftpClient.DirEntry> sessionFactory = new TestSftpSessionFactory();
TestSftpSessionFactory sessionFactory = new TestSftpSessionFactory();
FileTransferringMessageHandler<SftpClient.DirEntry> handler =
new FileTransferringMessageHandler<>(sessionFactory);
DefaultFileNameGenerator fGenerator = new DefaultFileNameGenerator();
@@ -127,6 +130,8 @@ public class SftpOutboundTests {
byte[] inFile = FileCopyUtils.copyToByteArray(file);
assertThat(new String(inFile)).isEqualTo("String data");
file.delete();
sessionFactory.destroy();
}
@Test
@@ -135,7 +140,7 @@ public class SftpOutboundTests {
if (file.exists()) {
file.delete();
}
SessionFactory<SftpClient.DirEntry> sessionFactory = new TestSftpSessionFactory();
TestSftpSessionFactory sessionFactory = new TestSftpSessionFactory();
FileTransferringMessageHandler<SftpClient.DirEntry> handler =
new FileTransferringMessageHandler<>(sessionFactory);
DefaultFileNameGenerator fGenerator = new DefaultFileNameGenerator();
@@ -151,6 +156,8 @@ public class SftpOutboundTests {
byte[] inFile = FileCopyUtils.copyToByteArray(file);
assertThat(new String(inFile)).isEqualTo("byte[] data");
file.delete();
sessionFactory.destroy();
}
@Test //INT-2275
@@ -225,7 +232,7 @@ public class SftpOutboundTests {
@ParameterizedTest
@ValueSource(booleans = {true, false})
public void testSharedSession(boolean sharedSession) throws IOException {
public void testSharedSession(boolean sharedSession) throws Exception {
try (SshServer server = SshServer.setUpDefaultServer()) {
server.setPasswordAuthenticator((arg0, arg1, arg2) -> true);
server.setPort(0);
@@ -253,6 +260,8 @@ public class SftpOutboundTests {
assertThat(TestUtils.getPropertyValue(s2, "sftpClient"))
.isNotSameAs(TestUtils.getPropertyValue(s1, "sftpClient"));
}
f.destroy();
}
}

View File

@@ -86,6 +86,7 @@ import static org.mockito.Mockito.verify;
/**
* @author Artem Bilan
* @author Gary Russell
* @author Darryl Smith
*
* @since 3.0
*/
@@ -684,7 +685,7 @@ public class SftpServerOutboundTests extends SftpTestSupport {
}
@Test
public void testSessionExists() throws IOException {
public void testSessionExists() throws Exception {
DefaultSftpSessionFactory sessionFactory = new DefaultSftpSessionFactory();
sessionFactory.setHost("localhost");
sessionFactory.setPort(port);
@@ -702,6 +703,8 @@ public class SftpServerOutboundTests extends SftpTestSupport {
.isThrownBy(() -> session.exists("any"))
.withRootCauseInstanceOf(IOException.class)
.withStackTraceContaining("canonicalPath(any) client is closed");
sessionFactory.destroy();
}
@SuppressWarnings("unused")

View File

@@ -1,5 +1,5 @@
/*
* Copyright 2014-2023 the original author or authors.
* Copyright 2014-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -54,6 +54,7 @@ import static org.mockito.Mockito.mock;
/**
* @author Gary Russell
* @author Artem Bilan
* @author Darryl Smith
* @since 4.1
*/
@SpringJUnitConfig
@@ -138,7 +139,7 @@ public class SftpRemoteFileTemplateTests extends SftpTestSupport {
}
@Test
public void renameWithOldSftpVersion() {
public void renameWithOldSftpVersion() throws Exception {
DefaultSftpSessionFactory factory = new DefaultSftpSessionFactory(false);
factory.setHost("localhost");
factory.setPort(port);
@@ -162,6 +163,8 @@ public class SftpRemoteFileTemplateTests extends SftpTestSupport {
"sftpSource/subSftpSource/subSftpSource1.txt"));
oldVersionSession.close();
factory.destroy();
}
@Test

View File

@@ -1,5 +1,5 @@
/*
* Copyright 2014-2023 the original author or authors.
* Copyright 2014-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -50,6 +50,7 @@ import static org.assertj.core.api.Assertions.assertThat;
* @author Gary Russell
* @author David Liu
* @author Artem Bilan
* @author Darryl Smith
*
* @since 4.1
*
@@ -76,6 +77,8 @@ public class SftpServerTests {
f.setAllowUnknownKeys(true);
Session<SftpClient.DirEntry> session = f.getSession();
doTest(server, session);
f.destroy();
}
}
@@ -111,6 +114,8 @@ public class SftpServerTests {
f.setPrivateKeyPassphrase(passphrase);
Session<SftpClient.DirEntry> session = f.getSession();
doTest(server, session);
f.destroy();
}
}

View File

@@ -1,5 +1,5 @@
/*
* Copyright 2014-2023 the original author or authors.
* Copyright 2014-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -30,6 +30,7 @@ import org.apache.sshd.client.SshClient;
import org.apache.sshd.client.auth.password.PasswordIdentityProvider;
import org.apache.sshd.client.channel.ClientChannel;
import org.apache.sshd.client.keyverifier.AcceptAllServerKeyVerifier;
import org.apache.sshd.client.session.ClientSession;
import org.apache.sshd.common.SshException;
import org.apache.sshd.server.SshServer;
import org.apache.sshd.server.keyprovider.SimpleGeneratorHostKeyProvider;
@@ -50,6 +51,7 @@ import static org.awaitility.Awaitility.await;
* @author Gary Russell
* @author Artem Bilan
* @author Auke Zaaiman
* @author Darryl Smith
*
* @since 3.0.2
*/
@@ -101,11 +103,13 @@ public class SftpSessionFactoryTests {
}
assertThat(server.getActiveSessions().size()).isEqualTo(0);
f.destroy();
}
}
@Test
public void concurrentGetSessionDoesntCauseFailure() throws IOException {
public void concurrentGetSessionDoesntCauseFailure() throws Exception {
try (SshServer server = SshServer.setUpDefaultServer()) {
server.setPasswordAuthenticator((arg0, arg1, arg2) -> true);
server.setPort(0);
@@ -134,11 +138,13 @@ public class SftpSessionFactoryTests {
.isNotEqualTo(concurrentSessions.get(2));
assertThat(concurrentSessions.get(1)).isNotEqualTo(concurrentSessions.get(2));
sftpSessionFactory.destroy();
}
}
@Test
void externallyProvidedSshClientShouldNotHaveItsConfigurationOverwritten() throws IOException {
void externallyProvidedSshClientShouldNotHaveItsConfigurationOverwritten() throws Exception {
try (SshServer server = SshServer.setUpDefaultServer()) {
server.setPasswordAuthenticator((arg0, arg1, arg2) -> true);
server.setPort(0);
@@ -156,11 +162,13 @@ public class SftpSessionFactoryTests {
sftpSessionFactory.setUser("user");
assertThatNoException().isThrownBy(sftpSessionFactory::getSession);
sftpSessionFactory.destroy();
}
}
@Test
void concurrentSessionListDoesntCauseFailure() throws IOException {
void concurrentSessionListDoesntCauseFailure() throws Exception {
try (SshServer server = SshServer.setUpDefaultServer()) {
server.setPasswordAuthenticator((arg0, arg1, arg2) -> true);
server.setPort(0);
@@ -192,11 +200,13 @@ public class SftpSessionFactoryTests {
.toList();
assertThat(dirEntries).hasSize(10);
sftpSessionFactory.destroy();
}
}
@Test
void customTimeoutIsApplied() throws IOException {
void customTimeoutIsApplied() throws Exception {
try (SshServer server = SshServer.setUpDefaultServer()) {
server.setPasswordAuthenticator((arg0, arg1, arg2) -> true);
server.setPort(0);
@@ -215,7 +225,39 @@ public class SftpSessionFactoryTests {
ClientChannel clientChannel = sftpSessionFactory.getSession().getClientInstance().getClientChannel();
assertThat(AbstractSftpClient.SFTP_CLIENT_CMD_TIMEOUT.getRequired(clientChannel)).hasSeconds(15);
sftpSessionFactory.destroy();
}
}
@Test
void clientSessionIsClosedOnSessionClose() throws Exception {
try (SshServer server = SshServer.setUpDefaultServer()) {
server.setPasswordAuthenticator((arg0, arg1, arg2) -> true);
server.setPort(0);
server.setKeyPairProvider(new SimpleGeneratorHostKeyProvider(new File("hostkey.ser").toPath()));
server.setSubsystemFactories(Collections.singletonList(new SftpSubsystemFactory()));
server.start();
DefaultSftpSessionFactory sftpSessionFactory = new DefaultSftpSessionFactory();
sftpSessionFactory.setHost("localhost");
sftpSessionFactory.setPort(server.getPort());
sftpSessionFactory.setUser("user");
sftpSessionFactory.setPassword("pass");
sftpSessionFactory.setAllowUnknownKeys(true);
SftpSession session = sftpSessionFactory.getSession();
ClientSession clientSession = session.getClientInstance().getClientSession();
assertThat(session.isOpen()).isTrue();
assertThat(clientSession.isOpen()).isTrue();
session.close();
assertThat(session.isOpen()).isFalse();
assertThat(clientSession.isClosed()).isTrue();
sftpSessionFactory.destroy();
}
}
}