Skip to content

Commit 8075e15

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 78becd5 commit 8075e15

File tree

3 files changed

+52
-16
lines changed

3 files changed

+52
-16
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
@@ -559,7 +559,7 @@ else if (FileExistsMode.APPEND.equals(mode)) {
559559
session.append(stream, tempFilePath);
560560
}
561561
else {
562-
if (exists(remoteFilePath)) {
562+
if (session.exists(remoteFilePath)) {
563563
if (FileExistsMode.FAIL.equals(mode)) {
564564
throw new MessagingException(
565565
"The destination file already exists at '" + remoteFilePath + "'.");

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

Lines changed: 7 additions & 9 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.
@@ -30,6 +30,7 @@
3030
import static org.junit.Assert.fail;
3131
import static org.mockito.ArgumentMatchers.any;
3232
import static org.mockito.ArgumentMatchers.anyString;
33+
import static org.mockito.BDDMockito.willReturn;
3334
import static org.mockito.Mockito.doAnswer;
3435
import static org.mockito.Mockito.mock;
3536
import static org.mockito.Mockito.times;
@@ -832,14 +833,10 @@ public void testPutExists() throws Exception {
832833
SessionFactory<TestLsEntry> sessionFactory = mock(SessionFactory.class);
833834
@SuppressWarnings("unchecked")
834835
Session<TestLsEntry> session = mock(Session.class);
835-
RemoteFileTemplate<TestLsEntry> template = new RemoteFileTemplate<TestLsEntry>(sessionFactory) {
836-
837-
@Override
838-
public boolean exists(String path) {
839-
return true;
840-
}
841-
842-
};
836+
willReturn(Boolean.TRUE)
837+
.given(session)
838+
.exists(anyString());
839+
RemoteFileTemplate<TestLsEntry> template = new RemoteFileTemplate<>(sessionFactory);
843840
template.setRemoteDirectoryExpression(new LiteralExpression("foo/"));
844841
template.setBeanFactory(mock(BeanFactory.class));
845842
template.afterPropertiesSet();
@@ -967,6 +964,7 @@ public void testMputRecursive() throws Exception {
967964
}
968965

969966
@Test
967+
@SuppressWarnings("unchecked")
970968
public void testMputCollection() throws Exception {
971969
@SuppressWarnings("unchecked")
972970
SessionFactory<TestLsEntry> sessionFactory = mock(SessionFactory.class);

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

Lines changed: 43 additions & 5 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.
@@ -16,11 +16,15 @@
1616

1717
package org.springframework.integration.sftp.session;
1818

19+
import static org.hamcrest.CoreMatchers.containsString;
20+
import static org.hamcrest.CoreMatchers.instanceOf;
1921
import static org.hamcrest.Matchers.containsInAnyOrder;
2022
import static org.junit.Assert.assertEquals;
2123
import static org.junit.Assert.assertFalse;
2224
import static org.junit.Assert.assertThat;
2325
import static org.junit.Assert.assertTrue;
26+
import static org.junit.Assert.fail;
27+
import static org.mockito.Mockito.mock;
2428

2529
import java.util.Arrays;
2630
import java.util.List;
@@ -29,6 +33,7 @@
2933
import org.junit.Test;
3034
import org.junit.runner.RunWith;
3135

36+
import org.springframework.beans.factory.BeanFactory;
3237
import org.springframework.beans.factory.annotation.Autowired;
3338
import org.springframework.context.annotation.Bean;
3439
import org.springframework.context.annotation.Configuration;
@@ -38,11 +43,13 @@
3843
import org.springframework.integration.file.remote.SessionCallbackWithoutResult;
3944
import org.springframework.integration.file.remote.session.CachingSessionFactory;
4045
import org.springframework.integration.file.remote.session.SessionFactory;
46+
import org.springframework.integration.file.support.FileExistsMode;
4147
import org.springframework.integration.sftp.SftpTestSupport;
48+
import org.springframework.messaging.MessageDeliveryException;
49+
import org.springframework.messaging.MessagingException;
4250
import org.springframework.messaging.support.GenericMessage;
4351
import org.springframework.test.annotation.DirtiesContext;
44-
import org.springframework.test.context.ContextConfiguration;
45-
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
52+
import org.springframework.test.context.junit4.SpringRunner;
4653

4754
import com.jcraft.jsch.ChannelSftp;
4855
import com.jcraft.jsch.ChannelSftp.LsEntry;
@@ -51,11 +58,12 @@
5158

5259
/**
5360
* @author Gary Russell
61+
* @author Artem Bilan
62+
*
5463
* @since 4.1
5564
*
5665
*/
57-
@ContextConfiguration
58-
@RunWith(SpringJUnit4ClassRunner.class)
66+
@RunWith(SpringRunner.class)
5967
@DirtiesContext
6068
public class SftpRemoteFileTemplateTests extends SftpTestSupport {
6169

@@ -67,9 +75,13 @@ public void testINT3412AppendStatRmdir() {
6775
SftpRemoteFileTemplate template = new SftpRemoteFileTemplate(sessionFactory);
6876
DefaultFileNameGenerator fileNameGenerator = new DefaultFileNameGenerator();
6977
fileNameGenerator.setExpression("'foobar.txt'");
78+
fileNameGenerator.setBeanFactory(mock(BeanFactory.class));
7079
template.setFileNameGenerator(fileNameGenerator);
7180
template.setRemoteDirectoryExpression(new LiteralExpression("foo/"));
7281
template.setUseTemporaryFileName(false);
82+
template.setBeanFactory(mock(BeanFactory.class));
83+
template.afterPropertiesSet();
84+
7385
template.execute(session -> {
7486
session.mkdir("foo/");
7587
return session.mkdir("foo/bar/");
@@ -101,6 +113,32 @@ public void testINT3412AppendStatRmdir() {
101113
assertFalse(template.exists("foo"));
102114
}
103115

116+
@Test
117+
public void testNoDeadLockOnSend() {
118+
CachingSessionFactory<LsEntry> cachingSessionFactory = new CachingSessionFactory<>(sessionFactory(), 1);
119+
SftpRemoteFileTemplate template = new SftpRemoteFileTemplate(cachingSessionFactory);
120+
template.setRemoteDirectoryExpression(new LiteralExpression(""));
121+
template.setBeanFactory(mock(BeanFactory.class));
122+
template.setUseTemporaryFileName(false);
123+
DefaultFileNameGenerator fileNameGenerator = new DefaultFileNameGenerator();
124+
fileNameGenerator.setExpression("'test.file'");
125+
fileNameGenerator.setBeanFactory(mock(BeanFactory.class));
126+
template.setFileNameGenerator(fileNameGenerator);
127+
template.afterPropertiesSet();
128+
129+
template.send(new GenericMessage<>(""));
130+
131+
try {
132+
template.send(new GenericMessage<>(""), FileExistsMode.FAIL);
133+
fail("Expected exception");
134+
}
135+
catch (MessageDeliveryException e) {
136+
assertThat(e.getCause(), instanceOf(MessagingException.class));
137+
assertThat(e.getMessage(), containsString("he destination file already exists at 'test.file'."));
138+
}
139+
cachingSessionFactory.destroy();
140+
}
141+
104142
@Configuration
105143
public static class Config {
106144

0 commit comments

Comments
 (0)