Spring/spring-integration
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

INT-4175: Fix SftpSessionFactory Race Condition

Browse Source 
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.
This commit is contained in:
Artem Bilan
2016-11-29 16:35:12 -05:00
committed by Gary Russell
parent ce2092c267
commit ad08d9add0
2 changed files with 45 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
spring-integration-sftp/src/main/java/org/springframework/integration/sftp/session/DefaultSftpSessionFactory.java
View File

@@ -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<LsEntry>, 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 {

46
spring-integration-sftp/src/test/java/org/springframework/integration/sftp/outbound/SftpOutboundTests.java
View File

@@ -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<File>(srcFile));
handler.handleMessage(new GenericMessage<>(srcFile));
assertTrue("destination file was not created", destFile.exists());
}
@@ -112,7 +114,7 @@ public class SftpOutboundTests {
file.delete();
}
SessionFactory<LsEntry> sessionFactory = new TestSftpSessionFactory();
FileTransferringMessageHandler<LsEntry> handler = new FileTransferringMessageHandler<LsEntry>(sessionFactory);
FileTransferringMessageHandler<LsEntry> 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<com.jcraft.jsch.Session> ctor = com.jcraft.jsch.Session.class.getDeclaredConstructor(JSch.class, String.class, String.class, int.class);
Constructor<com.jcraft.jsch.Session> 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<com.jcraft.jsch.Session> ctor = com.jcraft.jsch.Session.class.getDeclaredConstructor(JSch.class, String.class, String.class, int.class);
Constructor<com.jcraft.jsch.Session> 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();