Skip to content

Commit 5666051

Browse files
committed
GH-11277: Fix local directory creation race in remote file GET
Fixes: #11277 `generateLocalDirectory()` used a non-atomic `exists()`/`mkdirs()` sequence. With a `local-directory-expression` the eager `setupLocalDirectory()` in `afterPropertiesSet()` is skipped, since it only runs for a `ValueExpression`, so the directory is created per message on the calling thread. Concurrent messages resolving to the same, not-yet-existing local directory raced on `File.mkdirs()`: the losing threads received `false` and failed with `IllegalArgumentException: Failed to make local directory: [...]` although the directory existed at that point. * Use the idempotent and concurrency-safe `Files.createDirectories()` instead, retaining the exception type and message and propagating a genuine creation failure as cause This mirrors the fix applied to `FileWritingMessageHandler` in GH-11254. Attaching the `IOException` as cause moves the root cause of the reported `IllegalArgumentException` from itself to the `FileSystemException` raised by the failed creation. `FtpServerOutboundTests` and `SftpServerOutboundTests` pinned the old position via `hasRootCauseInstanceOf(IllegalArgumentException)`, which held only because the previous `Assert.isTrue()` left the chain empty. * Assert the `IllegalArgumentException` and its message where they now occur in the chain, and keep a root cause type assertion for the `FileSystemException` that the failed creation contributes Signed-off-by: Jiwoo Lee <dlwldn30@naver.com>
1 parent fe5e7d8 commit 5666051

4 files changed

Lines changed: 82 additions & 5 deletions

File tree

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.io.IOException;
2424
import java.io.OutputStream;
2525
import java.io.UncheckedIOException;
26+
import java.nio.file.Files;
2627
import java.util.ArrayList;
2728
import java.util.Arrays;
2829
import java.util.Collection;
@@ -77,6 +78,7 @@
7778
* @author Artem Bilan
7879
* @author Mauro Molinari
7980
* @author Jooyoung Pyoung
81+
* @author Jiwoo Lee
8082
*
8183
* @since 2.1
8284
*/
@@ -1428,7 +1430,12 @@ private File generateLocalDirectory(Message<?> message, @Nullable String remoteD
14281430
File localDir = ExpressionUtils.expressionToFile(this.localDirectoryExpression, evaluationContext, message,
14291431
"Local Directory");
14301432
if (!localDir.exists()) {
1431-
Assert.isTrue(localDir.mkdirs(), () -> "Failed to make local directory: " + localDir);
1433+
try {
1434+
Files.createDirectories(localDir.toPath());
1435+
}
1436+
catch (IOException ex) {
1437+
throw new IllegalArgumentException("Failed to make local directory: " + localDir, ex);
1438+
}
14321439
}
14331440
return localDir;
14341441
}

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

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,18 @@
2323
import java.io.IOException;
2424
import java.io.InputStream;
2525
import java.io.OutputStream;
26+
import java.nio.charset.StandardCharsets;
2627
import java.nio.file.Files;
2728
import java.util.ArrayList;
2829
import java.util.Calendar;
2930
import java.util.Collection;
3031
import java.util.Date;
3132
import java.util.List;
3233
import java.util.Map;
34+
import java.util.concurrent.CountDownLatch;
35+
import java.util.concurrent.ExecutorService;
36+
import java.util.concurrent.Executors;
37+
import java.util.concurrent.TimeUnit;
3338
import java.util.concurrent.atomic.AtomicReference;
3439

3540
import org.junit.jupiter.api.Test;
@@ -38,6 +43,7 @@
3843

3944
import org.springframework.expression.common.LiteralExpression;
4045
import org.springframework.expression.spel.standard.SpelExpressionParser;
46+
import org.springframework.integration.expression.FunctionExpression;
4147
import org.springframework.integration.file.FileHeaders;
4248
import org.springframework.integration.file.filters.AbstractSimplePatternFileListFilter;
4349
import org.springframework.integration.file.remote.AbstractFileInfo;
@@ -72,6 +78,7 @@
7278
* @author Liu Jiong
7379
* @author Artem Bilan
7480
* @author Jooyoung Pyoung
81+
* @author Jiwoo Lee
7582
*
7683
* @since 2.1
7784
*/
@@ -809,6 +816,67 @@ public void read(String source, OutputStream outputStream) throws IOException {
809816
out.delete();
810817
}
811818

819+
@Test
820+
public void testGetConcurrentCreateSameLocalDirectory(@TempDir File localRoot) throws Exception {
821+
SessionFactory sessionFactory = mock(SessionFactory.class);
822+
TestRemoteFileOutboundGateway gw = new TestRemoteFileOutboundGateway(sessionFactory, "get", "payload");
823+
// A non-ValueExpression skips the eager setupLocalDirectory() in afterPropertiesSet(),
824+
// so the directory is created per message on the calling thread.
825+
gw.setLocalDirectoryExpression(
826+
new FunctionExpression<Message<?>>((message) -> message.getHeaders().get("localDir")));
827+
gw.afterPropertiesSet();
828+
when(sessionFactory.getSession()).thenReturn(new TestSession() {
829+
830+
@Override
831+
public TestLsEntry[] list(String path) {
832+
return new TestLsEntry[] {
833+
new TestLsEntry(path, 1234, false, false, 12345, "-rw-r--r--")
834+
};
835+
}
836+
837+
@Override
838+
public void read(String source, OutputStream outputStream) throws IOException {
839+
outputStream.write("testfile".getBytes(StandardCharsets.UTF_8));
840+
}
841+
842+
});
843+
844+
int concurrency = 8;
845+
ExecutorService executorService = Executors.newFixedThreadPool(concurrency);
846+
AtomicReference<Throwable> failure = new AtomicReference<>();
847+
try {
848+
for (int round = 0; round < 50; round++) {
849+
File localDirectory = new File(localRoot, "local-" + round);
850+
CountDownLatch getsDone = new CountDownLatch(concurrency);
851+
for (int i = 0; i < concurrency; i++) {
852+
// A distinct remote file per thread; only the destination directory is shared.
853+
Message<String> message = MessageBuilder.withPayload("f" + i)
854+
.setHeader("localDir", localDirectory.getAbsolutePath())
855+
.build();
856+
executorService.execute(() -> {
857+
try {
858+
gw.handleRequestMessage(message);
859+
}
860+
catch (Throwable ex) { // NOSONAR - the race surfaces as an unchecked exception
861+
failure.compareAndSet(null, ex);
862+
}
863+
finally {
864+
getsDone.countDown();
865+
}
866+
});
867+
}
868+
assertThat(getsDone.await(10, TimeUnit.SECONDS)).isTrue();
869+
assertThat(failure.get()).isNull();
870+
for (int i = 0; i < concurrency; i++) {
871+
assertThat(new File(localDirectory, "f" + i)).exists();
872+
}
873+
}
874+
}
875+
finally {
876+
executorService.shutdownNow();
877+
}
878+
}
879+
812880
@Test
813881
public void testRm() throws Exception {
814882
SessionFactory sessionFactory = mock(SessionFactory.class);

spring-integration-ftp/src/test/java/org/springframework/integration/ftp/outbound/FtpServerOutboundTests.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.io.IOException;
2424
import java.io.InputStream;
2525
import java.net.InetSocketAddress;
26+
import java.nio.file.FileSystemException;
2627
import java.util.Arrays;
2728
import java.util.Calendar;
2829
import java.util.List;
@@ -211,8 +212,8 @@ public void testGetWithRemove() {
211212
@Test
212213
public void testInt2866InvalidLocalDirectoryExpression() {
213214
assertThatCode(() -> this.invalidDirExpression.send(new GenericMessage<Object>("/ftpSource/ ftpSource1.txt")))
214-
.hasRootCauseInstanceOf(IllegalArgumentException.class)
215-
.hasStackTraceContaining("Failed to make local directory");
215+
.hasRootCauseInstanceOf(FileSystemException.class)
216+
.hasStackTraceContaining("java.lang.IllegalArgumentException: Failed to make local directory");
216217
}
217218

218219
@Test

spring-integration-sftp/src/test/java/org/springframework/integration/sftp/outbound/SftpServerOutboundTests.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import java.io.PipedInputStream;
2626
import java.io.PipedOutputStream;
2727
import java.io.UncheckedIOException;
28+
import java.nio.file.FileSystemException;
2829
import java.util.ArrayList;
2930
import java.util.List;
3031
import java.util.concurrent.CountDownLatch;
@@ -191,8 +192,8 @@ public void testInt2866InvalidLocalDirectoryExpression() {
191192
assertThatExceptionOfType(Exception.class)
192193
.isThrownBy(() ->
193194
this.invalidDirExpression.send(new GenericMessage<Object>("sftpSource/ sftpSource1.txt")))
194-
.withRootCauseInstanceOf(IllegalArgumentException.class)
195-
.withStackTraceContaining("Failed to make local directory");
195+
.withRootCauseInstanceOf(FileSystemException.class)
196+
.withStackTraceContaining("java.lang.IllegalArgumentException: Failed to make local directory");
196197
}
197198

198199
@Test

0 commit comments

Comments
 (0)