From ad08d9add09a8f9fd5d2d3b8087059ec7bcda0d3 Mon Sep 17 00:00:00 2001 From: Artem Bilan Date: Tue, 29 Nov 2016 16:35:12 -0500 Subject: [PATCH] INT-4175: Fix SftpSessionFactory Race Condition JIRA: https://jira.spring.io/browse/INT-4175 Fixes GH-1980 (https://github.com/spring-projects/spring-integration/issues/1980) When the `isSharedSession` is used for the `DefaultSftpSessionFactory`, there is some race condition window when we can call the target `this.jschSession.connect()` several times and end up with the session is already connected. Wrap `sftpSession.connect()` to the `this.sharedSessionLock.readLock().lock()` when `isSharedSession` to protect from that race condition. Note: there is no test for this change because it is pretty tricky to build barriers between threads for this kind of race conditions. Especially after introduction `this.sharedSessionLock.readLock().lock()` around guilty code **Cherry-pick to 4.3.x & 4.2.x** Move `sharedJschSession` connect logic to the existing locking block Fix mock test to reflect the current ConnectionFactory state Conflicts: spring-integration-sftp/src/test/java/org/springframework/integration/sftp/outbound/SftpOutboundTests.java Resolved. --- .../session/DefaultSftpSessionFactory.java | 7 +++ .../sftp/outbound/SftpOutboundTests.java | 46 +++++++++++++++---- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/spring-integration-sftp/src/main/java/org/springframework/integration/sftp/session/DefaultSftpSessionFactory.java b/spring-integration-sftp/src/main/java/org/springframework/integration/sftp/session/DefaultSftpSessionFactory.java index fc545f2c18..34751fcd17 100644 --- a/spring-integration-sftp/src/main/java/org/springframework/integration/sftp/session/DefaultSftpSessionFactory.java +++ b/spring-integration-sftp/src/main/java/org/springframework/integration/sftp/session/DefaultSftpSessionFactory.java @@ -34,6 +34,7 @@ import org.springframework.util.StringUtils; import com.jcraft.jsch.ChannelSftp.LsEntry; import com.jcraft.jsch.JSch; +import com.jcraft.jsch.JSchException; import com.jcraft.jsch.Proxy; import com.jcraft.jsch.SocketFactory; import com.jcraft.jsch.UIKeyboardInteractive; @@ -361,6 +362,12 @@ public class DefaultSftpSessionFactory implements SessionFactory, Share try { if (this.sharedJschSession == null || !this.sharedJschSession.isConnected()) { this.sharedJschSession = new JSchSessionWrapper(initJschSession()); + try { + this.sharedJschSession.getSession().connect(); + } + catch (JSchException e) { + throw new IllegalStateException("failed to connect", e); + } } } finally { diff --git a/spring-integration-sftp/src/test/java/org/springframework/integration/sftp/outbound/SftpOutboundTests.java b/spring-integration-sftp/src/test/java/org/springframework/integration/sftp/outbound/SftpOutboundTests.java index bf26106a67..ede12ef5d1 100644 --- a/spring-integration-sftp/src/test/java/org/springframework/integration/sftp/outbound/SftpOutboundTests.java +++ b/spring-integration-sftp/src/test/java/org/springframework/integration/sftp/outbound/SftpOutboundTests.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import static org.mockito.BDDMockito.willAnswer; import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; @@ -75,6 +76,7 @@ import com.jcraft.jsch.SftpATTRS; * @author Oleg Zhurakousky * @author Gary Russell * @author Gunnar Hillert + * @author Artem Bilan */ public class SftpOutboundTests { @@ -101,7 +103,7 @@ public class SftpOutboundTests { File destFile = new File(targetDir, srcFile.getName() + ".test"); destFile.deleteOnExit(); - handler.handleMessage(new GenericMessage(srcFile)); + handler.handleMessage(new GenericMessage<>(srcFile)); assertTrue("destination file was not created", destFile.exists()); } @@ -112,7 +114,7 @@ public class SftpOutboundTests { file.delete(); } SessionFactory sessionFactory = new TestSftpSessionFactory(); - FileTransferringMessageHandler handler = new FileTransferringMessageHandler(sessionFactory); + FileTransferringMessageHandler handler = new FileTransferringMessageHandler<>(sessionFactory); DefaultFileNameGenerator fGenerator = new DefaultFileNameGenerator(); fGenerator.setBeanFactory(mock(BeanFactory.class)); fGenerator.setExpression("'foo.txt'"); @@ -231,8 +233,21 @@ public class SftpOutboundTests { ctor.setAccessible(true); com.jcraft.jsch.Session jschSession1 = spy(ctor.newInstance(jsch, "foo", "host", 22)); com.jcraft.jsch.Session jschSession2 = spy(ctor.newInstance(jsch, "foo", "host", 22)); - new DirectFieldAccessor(jschSession1).setPropertyValue("isConnected", true); - new DirectFieldAccessor(jschSession2).setPropertyValue("isConnected", true); + + willAnswer(invocation -> { + new DirectFieldAccessor(jschSession1).setPropertyValue("isConnected", true); + return null; + }) + .given(jschSession1) + .connect(); + + willAnswer(invocation -> { + new DirectFieldAccessor(jschSession2).setPropertyValue("isConnected", true); + return null; + }) + .given(jschSession2) + .connect(); + when(jsch.getSession("foo", "host", 22)).thenReturn(jschSession1, jschSession2); final ChannelSftp channel1 = spy(new ChannelSftp()); doReturn("channel1").when(channel1).toString(); @@ -265,7 +280,8 @@ public class SftpOutboundTests { @Test public void testNotSharedSession() throws Exception { JSch jsch = spy(new JSch()); - Constructor ctor = com.jcraft.jsch.Session.class.getDeclaredConstructor(JSch.class, String.class, String.class, int.class); + Constructor ctor = + com.jcraft.jsch.Session.class.getDeclaredConstructor(JSch.class, String.class, String.class, int.class); ctor.setAccessible(true); com.jcraft.jsch.Session jschSession1 = spy(ctor.newInstance(jsch, "foo", "host", 22)); com.jcraft.jsch.Session jschSession2 = spy(ctor.newInstance(jsch, "foo", "host", 22)); @@ -294,12 +310,26 @@ public class SftpOutboundTests { @Test public void testSharedSessionCachedReset() throws Exception { JSch jsch = spy(new JSch()); - Constructor ctor = com.jcraft.jsch.Session.class.getDeclaredConstructor(JSch.class, String.class, String.class, int.class); + Constructor ctor = + com.jcraft.jsch.Session.class.getDeclaredConstructor(JSch.class, String.class, String.class, int.class); ctor.setAccessible(true); com.jcraft.jsch.Session jschSession1 = spy(ctor.newInstance(jsch, "foo", "host", 22)); com.jcraft.jsch.Session jschSession2 = spy(ctor.newInstance(jsch, "foo", "host", 22)); - new DirectFieldAccessor(jschSession1).setPropertyValue("isConnected", true); - new DirectFieldAccessor(jschSession2).setPropertyValue("isConnected", true); + + willAnswer(invocation -> { + new DirectFieldAccessor(jschSession1).setPropertyValue("isConnected", true); + return null; + }) + .given(jschSession1) + .connect(); + + willAnswer(invocation -> { + new DirectFieldAccessor(jschSession2).setPropertyValue("isConnected", true); + return null; + }) + .given(jschSession2) + .connect(); + when(jsch.getSession("foo", "host", 22)).thenReturn(jschSession1, jschSession2); final ChannelSftp channel1 = spy(new ChannelSftp()); doReturn("channel1").when(channel1).toString();