From 5ba1b9321f7de2737820241ba7bdc4fc4bd5a550 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** --- .../file/remote/RemoteFileTemplate.java | 4 +- .../session/SftpRemoteFileTemplateTests.java | 47 ++++++++++++++++--- 2 files changed, 42 insertions(+), 9 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 50e53c6a2a..a26d23b3dc 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 @@ -572,7 +572,7 @@ public class RemoteFileTemplate implements RemoteFileOperations, Initializ session.append(stream, 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-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 feaafc238f..af0d43f1c6 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-2019 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. @@ -17,6 +17,8 @@ package org.springframework.integration.sftp.session; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.mockito.Mockito.mock; import java.util.Arrays; import java.util.List; @@ -25,6 +27,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; @@ -34,11 +37,13 @@ import org.springframework.integration.file.remote.ClientCallbackWithoutResult; import org.springframework.integration.file.remote.SessionCallbackWithoutResult; import org.springframework.integration.file.remote.session.CachingSessionFactory; 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; @@ -47,11 +52,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 { @@ -63,15 +69,19 @@ public class SftpRemoteFileTemplateTests extends SftpTestSupport { 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/"); }); - template.append(new GenericMessage("foo")); - template.append(new GenericMessage("bar")); + template.append(new GenericMessage<>("foo")); + template.append(new GenericMessage<>("bar")); assertThat(template.exists("foo/foobar.txt")).isTrue(); template.executeWithClient((ClientCallbackWithoutResult) client -> { try { @@ -96,6 +106,29 @@ public class SftpRemoteFileTemplateTests extends SftpTestSupport { assertThat(template.exists("foo")).isFalse(); } + @Test + public void testNoDeadLockOnSend() { + 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<>("")); + + assertThatExceptionOfType(MessageDeliveryException.class) + .isThrownBy(() -> template.send(new GenericMessage<>(""), FileExistsMode.FAIL)) + .withCauseInstanceOf(MessagingException.class) + .withStackTraceContaining("he destination file already exists at 'test.file'."); + + cachingSessionFactory.destroy(); + } + @Configuration public static class Config {