From 977997e3bf3c7382729a2483a7314ca536e1e7e4 Mon Sep 17 00:00:00 2001 From: Artem Bilan Date: Wed, 30 Oct 2019 15:41:42 -0400 Subject: [PATCH] GH-3090: Add `logout() to `FtpSession.close()` (#3094) * GH-3090: Add `logout() to `FtpSession.close()` Fixes https://github.com/spring-projects/spring-integration/issues/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 --- .../integration/ftp/session/FtpSession.java | 2 +- .../ftp/session/SessionFactoryTests.java | 96 +++++++++++-------- 2 files changed, 57 insertions(+), 41 deletions(-) diff --git a/spring-integration-ftp/src/main/java/org/springframework/integration/ftp/session/FtpSession.java b/spring-integration-ftp/src/main/java/org/springframework/integration/ftp/session/FtpSession.java index 813cbff4aa..21b10bb1e0 100644 --- a/spring-integration-ftp/src/main/java/org/springframework/integration/ftp/session/FtpSession.java +++ b/spring-integration-ftp/src/main/java/org/springframework/integration/ftp/session/FtpSession.java @@ -154,6 +154,7 @@ public class FtpSession implements Session { if (this.readingRaw.get() && !finalizeRaw() && LOGGER.isWarnEnabled()) { LOGGER.warn("Finalize on readRaw() returned false for " + this); } + this.client.logout(); this.client.disconnect(); } catch (Exception e) { @@ -195,7 +196,6 @@ public class FtpSession implements Session { return this.client.removeDirectory(directory); } - @Override public boolean exists(String path) throws IOException { Assert.hasText(path, "'path' must not be empty"); diff --git a/spring-integration-ftp/src/test/java/org/springframework/integration/ftp/session/SessionFactoryTests.java b/spring-integration-ftp/src/test/java/org/springframework/integration/ftp/session/SessionFactoryTests.java index c282fda41f..4e55cad139 100644 --- a/spring-integration-ftp/src/test/java/org/springframework/integration/ftp/session/SessionFactoryTests.java +++ b/spring-integration-ftp/src/test/java/org/springframework/integration/ftp/session/SessionFactoryTests.java @@ -17,6 +17,8 @@ package org.springframework.integration.ftp.session; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.fail; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; @@ -30,8 +32,9 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import org.apache.commons.net.ftp.FTPClient; -import org.junit.Ignore; -import org.junit.Test; +import org.apache.commons.net.ftp.FTPFile; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; import org.mockito.Mockito; import org.springframework.integration.file.remote.session.CachingSessionFactory; @@ -44,14 +47,14 @@ import org.springframework.integration.util.PoolItemNotAvailableException; * @author Oleg Zhurakousky * @author Gunnar Hillert * @author Gary Russell + * @author Artem Bilan * */ -@SuppressWarnings({"rawtypes", "unchecked"}) -public class SessionFactoryTests { +class SessionFactoryTests { @Test - public void testTimeouts() throws Exception { + void testFtpClientInteraction() throws Exception { final FTPClient client = mock(FTPClient.class); DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory() { @@ -66,14 +69,19 @@ public class SessionFactoryTests { sessionFactory.setDataTimeout(789); doReturn(200).when(client).getReplyCode(); doReturn(true).when(client).login("foo", null); - sessionFactory.getSession(); + FtpSession session = sessionFactory.getSession(); verify(client).setConnectTimeout(123); verify(client).setDefaultTimeout(456); verify(client).setDataTimeout(789); + + session.close(); + + verify(client).logout(); + verify(client).disconnect(); } @Test - public void testWithControlEncoding() { + void testWithControlEncoding() { DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory(); sessionFactory.setControlEncoding("UTF-8"); assertThat(TestUtils.getPropertyValue(sessionFactory, "controlEncoding")) @@ -81,27 +89,29 @@ public class SessionFactoryTests { } @Test - public void testWithoutControlEncoding() { + void testWithoutControlEncoding() { DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory(); assertThat(TestUtils.getPropertyValue(sessionFactory, "controlEncoding")) .as("Expected controlEncoding value of 'ISO-8859-1'").isEqualTo("ISO-8859-1"); } - @Test(expected = IllegalArgumentException.class) - public void testEmptyControlEncoding() { + @Test + void testEmptyControlEncoding() { DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory(); - sessionFactory.setControlEncoding(""); + assertThatIllegalArgumentException() + .isThrownBy(() -> sessionFactory.setControlEncoding("")); } - @Test(expected = IllegalArgumentException.class) - public void testNullControlEncoding() { + @Test + void testNullControlEncoding() { DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory(); - sessionFactory.setControlEncoding(null); + assertThatIllegalArgumentException() + .isThrownBy(() -> sessionFactory.setControlEncoding(null)); } @Test - public void testClientModes() throws Exception { + void testClientModes() throws Exception { DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory(); Field[] fields = FTPClient.class.getDeclaredFields(); for (Field field : fields) { @@ -110,7 +120,7 @@ public class SessionFactoryTests { int clientMode = field.getInt(null); sessionFactory.setClientMode(clientMode); if (!(clientMode == FTPClient.ACTIVE_LOCAL_DATA_CONNECTION_MODE || - clientMode == FTPClient.PASSIVE_LOCAL_DATA_CONNECTION_MODE)) { + clientMode == FTPClient.PASSIVE_LOCAL_DATA_CONNECTION_MODE)) { fail("IllegalArgumentException expected"); } } @@ -123,74 +133,79 @@ public class SessionFactoryTests { @Test - public void testStaleConnection() throws Exception { - SessionFactory sessionFactory = Mockito.mock(SessionFactory.class); - Session sessionA = Mockito.mock(Session.class); - Session sessionB = Mockito.mock(Session.class); + @SuppressWarnings("unchecked") + void testStaleConnection() { + SessionFactory sessionFactory = Mockito.mock(SessionFactory.class); + Session sessionA = Mockito.mock(Session.class); + Session sessionB = Mockito.mock(Session.class); Mockito.when(sessionA.isOpen()).thenReturn(true); Mockito.when(sessionB.isOpen()).thenReturn(false); Mockito.when(sessionFactory.getSession()).thenReturn(sessionA); Mockito.when(sessionFactory.getSession()).thenReturn(sessionB); - CachingSessionFactory cachingFactory = new CachingSessionFactory(sessionFactory, 2); + CachingSessionFactory cachingFactory = new CachingSessionFactory<>(sessionFactory, 2); - Session firstSession = cachingFactory.getSession(); - Session secondSession = cachingFactory.getSession(); + Session firstSession = cachingFactory.getSession(); + Session secondSession = cachingFactory.getSession(); secondSession.close(); - Session nonStaleSession = cachingFactory.getSession(); + Session nonStaleSession = cachingFactory.getSession(); assertThat(TestUtils.getPropertyValue(nonStaleSession, "targetSession")) .isEqualTo(TestUtils.getPropertyValue(firstSession, "targetSession")); } @Test - public void testSameSessionFromThePool() throws Exception { - SessionFactory sessionFactory = Mockito.mock(SessionFactory.class); - Session session = Mockito.mock(Session.class); + @SuppressWarnings("unchecked") + void testSameSessionFromThePool() { + SessionFactory sessionFactory = Mockito.mock(SessionFactory.class); + Session session = Mockito.mock(Session.class); Mockito.when(sessionFactory.getSession()).thenReturn(session); - CachingSessionFactory cachingFactory = new CachingSessionFactory(sessionFactory, 2); + CachingSessionFactory cachingFactory = new CachingSessionFactory<>(sessionFactory, 2); - Session s1 = cachingFactory.getSession(); + Session s1 = cachingFactory.getSession(); s1.close(); - Session s2 = cachingFactory.getSession(); + Session s2 = cachingFactory.getSession(); s2.close(); assertThat(TestUtils.getPropertyValue(s2, "targetSession")) .isEqualTo(TestUtils.getPropertyValue(s1, "targetSession")); Mockito.verify(sessionFactory, Mockito.times(2)).getSession(); } - @Test (expected = PoolItemNotAvailableException.class) // timeout expire - public void testSessionWaitExpire() throws Exception { - SessionFactory sessionFactory = Mockito.mock(SessionFactory.class); - Session session = Mockito.mock(Session.class); + @Test + @SuppressWarnings("unchecked") + void testSessionWaitExpire() { + SessionFactory sessionFactory = Mockito.mock(SessionFactory.class); + Session session = Mockito.mock(Session.class); Mockito.when(sessionFactory.getSession()).thenReturn(session); - CachingSessionFactory cachingFactory = new CachingSessionFactory(sessionFactory, 2); + CachingSessionFactory cachingFactory = new CachingSessionFactory<>(sessionFactory, 2); cachingFactory.setSessionWaitTimeout(3000); cachingFactory.getSession(); cachingFactory.getSession(); - cachingFactory.getSession(); + + assertThatExceptionOfType(PoolItemNotAvailableException.class) // timeout expire + .isThrownBy(cachingFactory::getSession); } @Test - @Ignore - public void testConnectionLimit() throws Exception { + @Disabled + void testConnectionLimit() throws Exception { ExecutorService executor = Executors.newCachedThreadPool(); DefaultFtpSessionFactory sessionFactory = new DefaultFtpSessionFactory(); sessionFactory.setHost("192.168.28.143"); sessionFactory.setPassword("password"); sessionFactory.setUsername("user"); - final CachingSessionFactory factory = new CachingSessionFactory(sessionFactory, 2); + final CachingSessionFactory factory = new CachingSessionFactory<>(sessionFactory, 2); final Random random = new Random(); final AtomicInteger failures = new AtomicInteger(); for (int i = 0; i < 30; i++) { executor.execute(() -> { try { - Session session = factory.getSession(); + Session session = factory.getSession(); Thread.sleep(random.nextInt(5000)); session.close(); } @@ -205,4 +220,5 @@ public class SessionFactoryTests { assertThat(failures.get()).isEqualTo(0); } + }