Skip to content

Commit 977997e

Browse files
artembilangaryrussell
artembilan
authored and
garyrussell
committed
GH-3090: Add logout() to FtpSession.close()` (#3094)
* GH-3090: Add `logout() to `FtpSession.close()` Fixes #3090 Without `logout()` the FTP session is not closed at all, but just the connection is closed. Some FTP servers close those sessions eventually anyway, but some just leak with resources. **Cherry-pick to 5.1.x & 4.3.x** * * Migrate `SessionFactoryTests` to JUnit 5
1 parent 7aea76c commit 977997e

File tree

2 files changed

+57
-41
lines changed

2 files changed

+57
-41
lines changed

‎spring-integration-ftp/src/main/java/org/springframework/integration/ftp/session/FtpSession.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@ public void close() {
154154
if (this.readingRaw.get() && !finalizeRaw() && LOGGER.isWarnEnabled()) {
155155
LOGGER.warn("Finalize on readRaw() returned false for " + this);
156156
}
157+
this.client.logout();
157158
this.client.disconnect();
158159
}
159160
catch (Exception e) {
@@ -195,7 +196,6 @@ public boolean rmdir(String directory) throws IOException {
195196
return this.client.removeDirectory(directory);
196197
}
197198

198-
199199
@Override
200200
public boolean exists(String path) throws IOException {
201201
Assert.hasText(path, "'path' must not be empty");

‎spring-integration-ftp/src/test/java/org/springframework/integration/ftp/session/SessionFactoryTests.java‎

Lines changed: 56 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
package org.springframework.integration.ftp.session;
1818

1919
import static org.assertj.core.api.Assertions.assertThat;
20+
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
21+
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
2022
import static org.assertj.core.api.Assertions.fail;
2123
import static org.mockito.Mockito.doReturn;
2224
import static org.mockito.Mockito.mock;
@@ -30,8 +32,9 @@
3032
import java.util.concurrent.atomic.AtomicInteger;
3133

3234
import org.apache.commons.net.ftp.FTPClient;
33-
import org.junit.Ignore;
34-
import org.junit.Test;
35+
import org.apache.commons.net.ftp.FTPFile;
36+
import org.junit.jupiter.api.Disabled;
37+
import org.junit.jupiter.api.Test;
3538
import org.mockito.Mockito;
3639

3740
import org.springframework.integration.file.remote.session.CachingSessionFactory;
@@ -44,14 +47,14 @@
4447
* @author Oleg Zhurakousky
4548
* @author Gunnar Hillert
4649
* @author Gary Russell
50+
* @author Artem Bilan
4751
*
4852
*/
49-
@SuppressWarnings({"rawtypes", "unchecked"})
50-
public class SessionFactoryTests {
53+
class SessionFactoryTests {
5154

5255

5356
@Test
54-
public void testTimeouts() throws Exception {
57+
void testFtpClientInteraction() throws Exception {
5558
final FTPClient client = mock(FTPClient.class);
5659
DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory() {
5760

@@ -66,42 +69,49 @@ protected FTPClient createClientInstance() {
6669
sessionFactory.setDataTimeout(789);
6770
doReturn(200).when(client).getReplyCode();
6871
doReturn(true).when(client).login("foo", null);
69-
sessionFactory.getSession();
72+
FtpSession session = sessionFactory.getSession();
7073
verify(client).setConnectTimeout(123);
7174
verify(client).setDefaultTimeout(456);
7275
verify(client).setDataTimeout(789);
76+
77+
session.close();
78+
79+
verify(client).logout();
80+
verify(client).disconnect();
7381
}
7482

7583
@Test
76-
public void testWithControlEncoding() {
84+
void testWithControlEncoding() {
7785
DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory();
7886
sessionFactory.setControlEncoding("UTF-8");
7987
assertThat(TestUtils.getPropertyValue(sessionFactory, "controlEncoding"))
8088
.as("Expected controlEncoding value of 'UTF-8'").isEqualTo("UTF-8");
8189
}
8290

8391
@Test
84-
public void testWithoutControlEncoding() {
92+
void testWithoutControlEncoding() {
8593
DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory();
8694
assertThat(TestUtils.getPropertyValue(sessionFactory, "controlEncoding"))
8795
.as("Expected controlEncoding value of 'ISO-8859-1'").isEqualTo("ISO-8859-1");
8896
}
8997

90-
@Test(expected = IllegalArgumentException.class)
91-
public void testEmptyControlEncoding() {
98+
@Test
99+
void testEmptyControlEncoding() {
92100
DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory();
93-
sessionFactory.setControlEncoding("");
101+
assertThatIllegalArgumentException()
102+
.isThrownBy(() -> sessionFactory.setControlEncoding(""));
94103
}
95104

96-
@Test(expected = IllegalArgumentException.class)
97-
public void testNullControlEncoding() {
105+
@Test
106+
void testNullControlEncoding() {
98107
DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory();
99-
sessionFactory.setControlEncoding(null);
108+
assertThatIllegalArgumentException()
109+
.isThrownBy(() -> sessionFactory.setControlEncoding(null));
100110
}
101111

102112

103113
@Test
104-
public void testClientModes() throws Exception {
114+
void testClientModes() throws Exception {
105115
DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory();
106116
Field[] fields = FTPClient.class.getDeclaredFields();
107117
for (Field field : fields) {
@@ -110,7 +120,7 @@ public void testClientModes() throws Exception {
110120
int clientMode = field.getInt(null);
111121
sessionFactory.setClientMode(clientMode);
112122
if (!(clientMode == FTPClient.ACTIVE_LOCAL_DATA_CONNECTION_MODE ||
113-
clientMode == FTPClient.PASSIVE_LOCAL_DATA_CONNECTION_MODE)) {
123+
clientMode == FTPClient.PASSIVE_LOCAL_DATA_CONNECTION_MODE)) {
114124
fail("IllegalArgumentException expected");
115125
}
116126
}
@@ -123,74 +133,79 @@ public void testClientModes() throws Exception {
123133

124134

125135
@Test
126-
public void testStaleConnection() throws Exception {
127-
SessionFactory sessionFactory = Mockito.mock(SessionFactory.class);
128-
Session sessionA = Mockito.mock(Session.class);
129-
Session sessionB = Mockito.mock(Session.class);
136+
@SuppressWarnings("unchecked")
137+
void testStaleConnection() {
138+
SessionFactory<FTPFile> sessionFactory = Mockito.mock(SessionFactory.class);
139+
Session<FTPFile> sessionA = Mockito.mock(Session.class);
140+
Session<FTPFile> sessionB = Mockito.mock(Session.class);
130141
Mockito.when(sessionA.isOpen()).thenReturn(true);
131142
Mockito.when(sessionB.isOpen()).thenReturn(false);
132143

133144
Mockito.when(sessionFactory.getSession()).thenReturn(sessionA);
134145
Mockito.when(sessionFactory.getSession()).thenReturn(sessionB);
135146

136-
CachingSessionFactory cachingFactory = new CachingSessionFactory(sessionFactory, 2);
147+
CachingSessionFactory<FTPFile> cachingFactory = new CachingSessionFactory<>(sessionFactory, 2);
137148

138-
Session firstSession = cachingFactory.getSession();
139-
Session secondSession = cachingFactory.getSession();
149+
Session<FTPFile> firstSession = cachingFactory.getSession();
150+
Session<FTPFile> secondSession = cachingFactory.getSession();
140151
secondSession.close();
141-
Session nonStaleSession = cachingFactory.getSession();
152+
Session<FTPFile> nonStaleSession = cachingFactory.getSession();
142153
assertThat(TestUtils.getPropertyValue(nonStaleSession, "targetSession"))
143154
.isEqualTo(TestUtils.getPropertyValue(firstSession, "targetSession"));
144155
}
145156

146157
@Test
147-
public void testSameSessionFromThePool() throws Exception {
148-
SessionFactory sessionFactory = Mockito.mock(SessionFactory.class);
149-
Session session = Mockito.mock(Session.class);
158+
@SuppressWarnings("unchecked")
159+
void testSameSessionFromThePool() {
160+
SessionFactory<FTPFile> sessionFactory = Mockito.mock(SessionFactory.class);
161+
Session<FTPFile> session = Mockito.mock(Session.class);
150162
Mockito.when(sessionFactory.getSession()).thenReturn(session);
151163

152-
CachingSessionFactory cachingFactory = new CachingSessionFactory(sessionFactory, 2);
164+
CachingSessionFactory<FTPFile> cachingFactory = new CachingSessionFactory<>(sessionFactory, 2);
153165

154-
Session s1 = cachingFactory.getSession();
166+
Session<FTPFile> s1 = cachingFactory.getSession();
155167
s1.close();
156-
Session s2 = cachingFactory.getSession();
168+
Session<FTPFile> s2 = cachingFactory.getSession();
157169
s2.close();
158170
assertThat(TestUtils.getPropertyValue(s2, "targetSession"))
159171
.isEqualTo(TestUtils.getPropertyValue(s1, "targetSession"));
160172
Mockito.verify(sessionFactory, Mockito.times(2)).getSession();
161173
}
162174

163-
@Test (expected = PoolItemNotAvailableException.class) // timeout expire
164-
public void testSessionWaitExpire() throws Exception {
165-
SessionFactory sessionFactory = Mockito.mock(SessionFactory.class);
166-
Session session = Mockito.mock(Session.class);
175+
@Test
176+
@SuppressWarnings("unchecked")
177+
void testSessionWaitExpire() {
178+
SessionFactory<FTPFile> sessionFactory = Mockito.mock(SessionFactory.class);
179+
Session<FTPFile> session = Mockito.mock(Session.class);
167180
Mockito.when(sessionFactory.getSession()).thenReturn(session);
168181

169-
CachingSessionFactory cachingFactory = new CachingSessionFactory(sessionFactory, 2);
182+
CachingSessionFactory<FTPFile> cachingFactory = new CachingSessionFactory<>(sessionFactory, 2);
170183

171184
cachingFactory.setSessionWaitTimeout(3000);
172185

173186
cachingFactory.getSession();
174187
cachingFactory.getSession();
175-
cachingFactory.getSession();
188+
189+
assertThatExceptionOfType(PoolItemNotAvailableException.class) // timeout expire
190+
.isThrownBy(cachingFactory::getSession);
176191
}
177192

178193
@Test
179-
@Ignore
180-
public void testConnectionLimit() throws Exception {
194+
@Disabled
195+
void testConnectionLimit() throws Exception {
181196
ExecutorService executor = Executors.newCachedThreadPool();
182197
DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory();
183198
sessionFactory.setHost("192.168.28.143");
184199
sessionFactory.setPassword("password");
185200
sessionFactory.setUsername("user");
186-
final CachingSessionFactory factory = new CachingSessionFactory(sessionFactory, 2);
201+
final CachingSessionFactory<FTPFile> factory = new CachingSessionFactory<>(sessionFactory, 2);
187202

188203
final Random random = new Random();
189204
final AtomicInteger failures = new AtomicInteger();
190205
for (int i = 0; i < 30; i++) {
191206
executor.execute(() -> {
192207
try {
193-
Session session = factory.getSession();
208+
Session<FTPFile> session = factory.getSession();
194209
Thread.sleep(random.nextInt(5000));
195210
session.close();
196211
}
@@ -205,4 +220,5 @@ public void testConnectionLimit() throws Exception {
205220

206221
assertThat(failures.get()).isEqualTo(0);
207222
}
223+
208224
}

0 commit comments

Comments
 (0)