Skip to content

Commit e73e08b

Browse files
artembilangaryrussell
authored andcommitted
GH-3249: Fix RemoteFileTemplate dead lock in send
Fixes: #3249 When the `CachingSessionFactory` is configured with small enough pool and it is very likely that dead lock may happen when `RemoteFileTemplate.send()` is used. The problem happens when we reach the `RemoteFileTemplate.exists()` call which is done from the internal method called from already pulled from cache `Session` * Fix `RemoteFileTemplate` to use a `session.exists()` instead on the provided into the method `Session` * Demonstrate the problem in the `SftpRemoteFileTemplateTests.testNoDeadLockOnSend()` **Cherry-pick to 5.2.x, 5.1.x & 4.3.x** Fix RemoteFileOutboundGWTests for the proper mock
1 parent baac0a1 commit e73e08b

File tree

3 files changed

+50
-19
lines changed

3 files changed

+50
-19
lines changed

spring-integration-file/src/main/java/org/springframework/integration/file/remote/RemoteFileTemplate.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,7 @@ public void rename(final String fromPath, final String toPath) {
386386
public boolean get(Message<?> message, InputStreamCallback callback) {
387387
Assert.notNull(this.fileNameProcessor, "A 'fileNameExpression' is needed to use get");
388388
String remotePath = this.fileNameProcessor.processMessage(message);
389-
return this.get(remotePath, callback);
389+
return get(remotePath, callback);
390390
}
391391

392392
@Override
@@ -571,7 +571,7 @@ else if (FileExistsMode.APPEND.equals(mode)) {
571571
session.append(stream, tempFilePath);
572572
}
573573
else {
574-
if (exists(remoteFilePath)) {
574+
if (session.exists(remoteFilePath)) {
575575
if (FileExistsMode.FAIL.equals(mode)) {
576576
throw new MessagingException(
577577
"The destination file already exists at '" + remoteFilePath + "'.");

spring-integration-file/src/test/java/org/springframework/integration/file/remote/gateway/RemoteFileOutboundGatewayTests.java

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2019 the original author or authors.
2+
* Copyright 2002-2020 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -22,6 +22,7 @@
2222
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
2323
import static org.mockito.ArgumentMatchers.any;
2424
import static org.mockito.ArgumentMatchers.anyString;
25+
import static org.mockito.BDDMockito.willReturn;
2526
import static org.mockito.Mockito.doAnswer;
2627
import static org.mockito.Mockito.mock;
2728
import static org.mockito.Mockito.times;
@@ -804,14 +805,10 @@ public void testPutExists() throws Exception {
804805
SessionFactory<TestLsEntry> sessionFactory = mock(SessionFactory.class);
805806
@SuppressWarnings("unchecked")
806807
Session<TestLsEntry> session = mock(Session.class);
807-
RemoteFileTemplate<TestLsEntry> template = new RemoteFileTemplate<TestLsEntry>(sessionFactory) {
808-
809-
@Override
810-
public boolean exists(String path) {
811-
return true;
812-
}
813-
814-
};
808+
willReturn(Boolean.TRUE)
809+
.given(session)
810+
.exists(anyString());
811+
RemoteFileTemplate<TestLsEntry> template = new RemoteFileTemplate<>(sessionFactory);
815812
template.setRemoteDirectoryExpression(new LiteralExpression("foo/"));
816813
template.setBeanFactory(mock(BeanFactory.class));
817814
template.afterPropertiesSet();
@@ -932,7 +929,7 @@ public void testRemoteFileTemplateImmutability() {
932929
TestRemoteFileOutboundGateway gw = new TestRemoteFileOutboundGateway(template, "mput", null);
933930
assertThatIllegalStateException()
934931
.isThrownBy(() -> gw.setRemoteDirectoryExpression(new LiteralExpression("testRemoteDirectory")))
935-
.withMessageContaining("The 'remoteDirectoryExpression' must be set on the externally provided");
932+
.withMessageContaining("The 'remoteDirectoryExpression' must be set on the externally provided");
936933
}
937934

938935
@Test
@@ -942,6 +939,7 @@ public void testMputCollection() throws Exception {
942939
Session<TestLsEntry> session = mock(Session.class);
943940
TestRemoteFileOutboundGateway gw = new TestRemoteFileOutboundGateway(sessionFactory, "mput", "payload");
944941
gw.setRemoteDirectoryExpression(new LiteralExpression("foo/"));
942+
gw.setBeanFactory(mock(BeanFactory.class));
945943
gw.afterPropertiesSet();
946944
when(sessionFactory.getSession()).thenReturn(session);
947945
final AtomicReference<String> written = new AtomicReference<>();

spring-integration-sftp/src/test/java/org/springframework/integration/sftp/session/SftpRemoteFileTemplateTests.java

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2014-2019 the original author or authors.
2+
* Copyright 2014-2020 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -17,6 +17,8 @@
1717
package org.springframework.integration.sftp.session;
1818

1919
import static org.assertj.core.api.Assertions.assertThat;
20+
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
21+
import static org.mockito.Mockito.mock;
2022

2123
import java.util.Arrays;
2224
import java.util.List;
@@ -25,6 +27,7 @@
2527
import org.junit.Test;
2628
import org.junit.runner.RunWith;
2729

30+
import org.springframework.beans.factory.BeanFactory;
2831
import org.springframework.beans.factory.annotation.Autowired;
2932
import org.springframework.context.annotation.Bean;
3033
import org.springframework.context.annotation.Configuration;
@@ -34,11 +37,13 @@
3437
import org.springframework.integration.file.remote.SessionCallbackWithoutResult;
3538
import org.springframework.integration.file.remote.session.CachingSessionFactory;
3639
import org.springframework.integration.file.remote.session.SessionFactory;
40+
import org.springframework.integration.file.support.FileExistsMode;
3741
import org.springframework.integration.sftp.SftpTestSupport;
42+
import org.springframework.messaging.MessageDeliveryException;
43+
import org.springframework.messaging.MessagingException;
3844
import org.springframework.messaging.support.GenericMessage;
3945
import org.springframework.test.annotation.DirtiesContext;
40-
import org.springframework.test.context.ContextConfiguration;
41-
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
46+
import org.springframework.test.context.junit4.SpringRunner;
4247

4348
import com.jcraft.jsch.ChannelSftp;
4449
import com.jcraft.jsch.ChannelSftp.LsEntry;
@@ -47,11 +52,12 @@
4752

4853
/**
4954
* @author Gary Russell
55+
* @author Artem Bilan
56+
*
5057
* @since 4.1
5158
*
5259
*/
53-
@ContextConfiguration
54-
@RunWith(SpringJUnit4ClassRunner.class)
60+
@RunWith(SpringRunner.class)
5561
@DirtiesContext
5662
public class SftpRemoteFileTemplateTests extends SftpTestSupport {
5763

@@ -63,15 +69,19 @@ public void testINT3412AppendStatRmdir() {
6369
SftpRemoteFileTemplate template = new SftpRemoteFileTemplate(sessionFactory);
6470
DefaultFileNameGenerator fileNameGenerator = new DefaultFileNameGenerator();
6571
fileNameGenerator.setExpression("'foobar.txt'");
72+
fileNameGenerator.setBeanFactory(mock(BeanFactory.class));
6673
template.setFileNameGenerator(fileNameGenerator);
6774
template.setRemoteDirectoryExpression(new LiteralExpression("foo/"));
6875
template.setUseTemporaryFileName(false);
76+
template.setBeanFactory(mock(BeanFactory.class));
77+
template.afterPropertiesSet();
78+
6979
template.execute(session -> {
7080
session.mkdir("foo/");
7181
return session.mkdir("foo/bar/");
7282
});
73-
template.append(new GenericMessage<String>("foo"));
74-
template.append(new GenericMessage<String>("bar"));
83+
template.append(new GenericMessage<>("foo"));
84+
template.append(new GenericMessage<>("bar"));
7585
assertThat(template.exists("foo/foobar.txt")).isTrue();
7686
template.executeWithClient((ClientCallbackWithoutResult<ChannelSftp>) client -> {
7787
try {
@@ -96,6 +106,29 @@ public void testINT3412AppendStatRmdir() {
96106
assertThat(template.exists("foo")).isFalse();
97107
}
98108

109+
@Test
110+
public void testNoDeadLockOnSend() {
111+
CachingSessionFactory<LsEntry> cachingSessionFactory = new CachingSessionFactory<>(sessionFactory(), 1);
112+
SftpRemoteFileTemplate template = new SftpRemoteFileTemplate(cachingSessionFactory);
113+
template.setRemoteDirectoryExpression(new LiteralExpression(""));
114+
template.setBeanFactory(mock(BeanFactory.class));
115+
template.setUseTemporaryFileName(false);
116+
DefaultFileNameGenerator fileNameGenerator = new DefaultFileNameGenerator();
117+
fileNameGenerator.setExpression("'test.file'");
118+
fileNameGenerator.setBeanFactory(mock(BeanFactory.class));
119+
template.setFileNameGenerator(fileNameGenerator);
120+
template.afterPropertiesSet();
121+
122+
template.send(new GenericMessage<>(""));
123+
124+
assertThatExceptionOfType(MessageDeliveryException.class)
125+
.isThrownBy(() -> template.send(new GenericMessage<>(""), FileExistsMode.FAIL))
126+
.withCauseInstanceOf(MessagingException.class)
127+
.withStackTraceContaining("he destination file already exists at 'test.file'.");
128+
129+
cachingSessionFactory.destroy();
130+
}
131+
99132
@Configuration
100133
public static class Config {
101134

0 commit comments

Comments
 (0)