From c00c319acb414ce0ac6736fdc2c2f677b0cedd16 Mon Sep 17 00:00:00 2001 From: Artem Bilan Date: Sat, 18 Apr 2020 21:06:15 -0400 Subject: [PATCH] GH-3249: Fix RemoteFileTemplate dead lock in send Fixes: https://github.com/spring-projects/spring-integration/issues/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 --- .../file/remote/RemoteFileTemplate.java | 4 +- .../RemoteFileOutboundGatewayTests.java | 15 +++--- .../session/SftpRemoteFileTemplateTests.java | 50 ++++++++++++++++--- 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/spring-integration-file/src/main/java/org/springframework/integration/file/remote/RemoteFileTemplate.java b/spring-integration-file/src/main/java/org/springframework/integration/file/remote/RemoteFileTemplate.java index be03e567ca..52744b67cf 100644 --- a/spring-integration-file/src/main/java/org/springframework/integration/file/remote/RemoteFileTemplate.java +++ b/spring-integration-file/src/main/java/org/springframework/integration/file/remote/RemoteFileTemplate.java @@ -389,7 +389,7 @@ public class RemoteFileTemplate implements RemoteFileOperations, Initializ public boolean get(Message message, InputStreamCallback callback) { Assert.notNull(this.fileNameProcessor, "A 'fileNameExpression' is needed to use get"); String remotePath = this.fileNameProcessor.processMessage(message); - return this.get(remotePath, callback); + return get(remotePath, callback); } @Override @@ -545,7 +545,7 @@ public class RemoteFileTemplate implements RemoteFileOperations, Initializ session.append(inputStream, tempFilePath); } else { - if (exists(remoteFilePath)) { + if (session.exists(remoteFilePath)) { if (FileExistsMode.FAIL.equals(mode)) { throw new MessagingException( "The destination file already exists at '" + remoteFilePath + "'."); diff --git a/spring-integration-file/src/test/java/org/springframework/integration/file/remote/gateway/RemoteFileOutboundGatewayTests.java b/spring-integration-file/src/test/java/org/springframework/integration/file/remote/gateway/RemoteFileOutboundGatewayTests.java index 098c4ab733..31583dcde4 100644 --- a/spring-integration-file/src/test/java/org/springframework/integration/file/remote/gateway/RemoteFileOutboundGatewayTests.java +++ b/spring-integration-file/src/test/java/org/springframework/integration/file/remote/gateway/RemoteFileOutboundGatewayTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -28,6 +28,7 @@ import static org.junit.Assert.assertSame; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.BDDMockito.willReturn; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.doAnswer; @@ -845,14 +846,10 @@ public class RemoteFileOutboundGatewayTests { SessionFactory sessionFactory = mock(SessionFactory.class); @SuppressWarnings("unchecked") Session session = mock(Session.class); - RemoteFileTemplate template = new RemoteFileTemplate(sessionFactory) { - - @Override - public boolean exists(String path) { - return true; - } - - }; + willReturn(Boolean.TRUE) + .given(session) + .exists(anyString()); + RemoteFileTemplate template = new RemoteFileTemplate<>(sessionFactory); template.setRemoteDirectoryExpression(new LiteralExpression("foo/")); template.setBeanFactory(mock(BeanFactory.class)); template.afterPropertiesSet(); diff --git a/spring-integration-sftp/src/test/java/org/springframework/integration/sftp/session/SftpRemoteFileTemplateTests.java b/spring-integration-sftp/src/test/java/org/springframework/integration/sftp/session/SftpRemoteFileTemplateTests.java index 7909847681..0852609bb2 100644 --- a/spring-integration-sftp/src/test/java/org/springframework/integration/sftp/session/SftpRemoteFileTemplateTests.java +++ b/spring-integration-sftp/src/test/java/org/springframework/integration/sftp/session/SftpRemoteFileTemplateTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2017 the original author or authors. + * Copyright 2014-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,11 +16,15 @@ package org.springframework.integration.sftp.session; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; import java.io.IOException; import java.util.Arrays; @@ -30,6 +34,7 @@ import java.util.stream.Collectors; import org.junit.Test; import org.junit.runner.RunWith; +import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -40,11 +45,13 @@ import org.springframework.integration.file.remote.SessionCallbackWithoutResult; import org.springframework.integration.file.remote.session.CachingSessionFactory; import org.springframework.integration.file.remote.session.Session; import org.springframework.integration.file.remote.session.SessionFactory; +import org.springframework.integration.file.support.FileExistsMode; import org.springframework.integration.sftp.SftpTestSupport; +import org.springframework.messaging.MessageDeliveryException; +import org.springframework.messaging.MessagingException; import org.springframework.messaging.support.GenericMessage; import org.springframework.test.annotation.DirtiesContext; -import org.springframework.test.context.ContextConfiguration; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.test.context.junit4.SpringRunner; import com.jcraft.jsch.ChannelSftp; import com.jcraft.jsch.ChannelSftp.LsEntry; @@ -53,11 +60,12 @@ import com.jcraft.jsch.SftpException; /** * @author Gary Russell + * @author Artem Bilan + * * @since 4.1 * */ -@ContextConfiguration -@RunWith(SpringJUnit4ClassRunner.class) +@RunWith(SpringRunner.class) @DirtiesContext public class SftpRemoteFileTemplateTests extends SftpTestSupport { @@ -65,13 +73,17 @@ public class SftpRemoteFileTemplateTests extends SftpTestSupport { private CachingSessionFactory sessionFactory; @Test - public void testINT3412AppendStatRmdir() { + public void testINT3412AppendStatRmdir() throws Exception { SftpRemoteFileTemplate template = new SftpRemoteFileTemplate(sessionFactory); DefaultFileNameGenerator fileNameGenerator = new DefaultFileNameGenerator(); fileNameGenerator.setExpression("'foobar.txt'"); + fileNameGenerator.setBeanFactory(mock(BeanFactory.class)); template.setFileNameGenerator(fileNameGenerator); template.setRemoteDirectoryExpression(new LiteralExpression("foo/")); template.setUseTemporaryFileName(false); + template.setBeanFactory(mock(BeanFactory.class)); + template.afterPropertiesSet(); + template.execute(session -> { session.mkdir("foo/"); return session.mkdir("foo/bar/"); @@ -108,6 +120,32 @@ public class SftpRemoteFileTemplateTests extends SftpTestSupport { assertFalse(template.exists("foo")); } + @Test + public void testNoDeadLockOnSend() throws Exception { + CachingSessionFactory cachingSessionFactory = new CachingSessionFactory<>(sessionFactory(), 1); + SftpRemoteFileTemplate template = new SftpRemoteFileTemplate(cachingSessionFactory); + template.setRemoteDirectoryExpression(new LiteralExpression("")); + template.setBeanFactory(mock(BeanFactory.class)); + template.setUseTemporaryFileName(false); + DefaultFileNameGenerator fileNameGenerator = new DefaultFileNameGenerator(); + fileNameGenerator.setExpression("'test.file'"); + fileNameGenerator.setBeanFactory(mock(BeanFactory.class)); + template.setFileNameGenerator(fileNameGenerator); + template.afterPropertiesSet(); + + template.send(new GenericMessage<>("")); + + try { + template.send(new GenericMessage<>(""), FileExistsMode.FAIL); + fail("Expected exception"); + } + catch (MessageDeliveryException e) { + assertThat(e.getCause(), instanceOf(MessagingException.class)); + assertThat(e.getMessage(), containsString("he destination file already exists at 'test.file'.")); + } + cachingSessionFactory.destroy(); + } + @Configuration public static class Config {