GH-11253: Fix directory-creation race in `FileWritingMessageHandler` by tknall · Pull Request #11254 · spring-projects/spring-integration

tknall · 2026-08-10T10:40:51Z

FileWritingMessageHandler.validateDestinationDirectory() used a non-atomic
exists()/mkdirs() sequence. With a directory-expression, this code runs per
message on the calling thread, so concurrent messages resolving to the same,
not-yet-existing destination directory raced on File.mkdirs(): the losing threads
received false (the directory had been created concurrently in the meantime) and
failed with IllegalArgumentException: Destination directory [...] could not be created.
although the directory existed at that point.

Use the idempotent and concurrency-safe Files.createDirectories() instead. The
exception type and message are retained for backward compatibility; a genuine creation
failure now additionally carries the underlying IOException as cause.

Verified with the reproducer from GH-11253: fails within the first rounds before the
change, passes 3 x 500 rounds x 8 threads after it.

artembilan

Please, find my review in lines.

Thank you!

artembilan · 2026-08-10T13:59:02Z

+				CyclicBarrier barrier = new CyclicBarrier(concurrency);
+				List<Future<?>> results = new ArrayList<>();
+				for (int i = 0; i < concurrency; i++) {
+					Message<String> message = MessageBuilder.withPayload("foo")


No foo/bar language, please.
We slowly migrate existing code, but definitely don't want more of them in a new one.

Sorry, I was not aware of that.

Renamed the payload to "test data".

artembilan · 2026-08-10T15:26:49Z

+							.setHeader(FileHeaders.FILENAME, "file-" + i + ".txt")
+							.build();
+					results.add(executorService.submit(() -> {
+						barrier.await();


I'm not sure that we need a barrier...
Please, elaborate.

The barrier was there to line up all threads right before handleMessage() to maximize the chance of hitting the exists()/mkdirs() window.

However, I re-verified without it -> the test still failed consistently against the unpatched code in 10 out of 10 runs. So you are right, it is not needed; removed.

artembilan · 2026-08-10T15:31:23Z

+					}));
+				}
+				for (Future<?> result : results) {
+					result.get(10, TimeUnit.SECONDS);


I think we don't need such a complexity.
We just need one CountDownLatch to countDown in the Runnable after that handler.handleMessage().

Simplified as suggested: plain executorService.execute() with a single CountDownLatch counted down after handleMessage(), no futures involved.

artembilan · 2026-08-10T15:32:06Z

+				for (Future<?> result : results) {
+					result.get(10, TimeUnit.SECONDS);
+				}
+				assertThat(new File(directory, "file-0.txt")).exists();


This doesn't look finished.
I would expect here a loop to check that all requested files are created.

Added the assertion loop verifying that all requested files exist.

artembilan · 2026-08-10T15:34:19Z

+	@Test
+	public void concurrentWritesToSameNewDestinationDirectory() throws Exception {
+		FileWritingMessageHandler handler = new FileWritingMessageHandler(
+				new SpelExpressionParser().parseExpression("headers['directory']"));


We don't need SpEL parsing for such an expression style.
The FunctionExpression will do the trick for us:

new FunctionExpression<Message<?>>(message -> message.getHeaders().get("directory"))

Your are right. I wasn't aware of that. Thanks for the hint.
Done. Replaced by FunctionExpression.

Thank you!
You don't need to comment for every single my review line that you are agreed with me.
There is enough just one common: I'll see that we are on the same page with your next change set.
However you are free to discuss further if you think I'm wrong in my review.
You also don't need to squash all the commits after PR is opened.
I saw that question from my AI, so I believe yours also is trying to follow our CONTRIBUTION.md.
I guess that one has to be fixed respectively 😄

artembilan · 2026-08-10T15:37:14Z

+		FileWritingMessageHandler handler = new FileWritingMessageHandler(
+				new SpelExpressionParser().parseExpression("headers['directory']"));
+		handler.setBeanFactory(TEST_INTEGRATION_CONTEXT);
+		handler.setApplicationContext(new GenericApplicationContext());


The TEST_INTEGRATION_CONTEXT is already an ApplicationContext, so we don't need this new instance.
More over, please, double check if we need it at all.
I just removed same line of code in the @BeforeEach method of this class - and all tests have passed.

You are right - removed the setApplicationContext(new GenericApplicationContext()) line and double-checked that the test still behaves as intended...

…ssageHandler `validateDestinationDirectory()` used a non-atomic `exists()`/`mkdirs()` sequence. With a `directory-expression` this runs per message on the calling thread, so concurrent messages resolving to the same, not-yet-existing destination directory raced on `File.mkdirs()`: the losing threads received `false` and failed with `IllegalArgumentException: Destination directory [...] could not be created.` 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. Fixes: spring-projects#11253 Signed-off-by: Thomas Knall <thomas.knall@prime-sign.com>

artembilan

[ant:checkstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-file/src/test/java/org/springframework/integration/file/outbound/FileWritingMessageHandlerTests.java:54:1: Wrong lexicographical order for 'org.springframework.integration.channel.QueueChannel' import. Should be before 'org.springframework.integration.expression.FunctionExpression'. [ImportOrder]

Please, make sure you run ./gradlew check (or just for that specific module) before pushing changes upstream to the PR.

Thank you!

Signed-off-by: Thomas Knall <thomas.knall@prime-sign.com>

…file GET Fixes: spring-projects#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. * Treat `false` from `mkdirs()` as a failure only when the directory is still absent - the concurrency-tolerant idiom already used by `PropertiesPersistingMetadataStore` `Files.createDirectories()`, as used by `FileWritingMessageHandler` since spring-projectsGH-11254, is race-tolerant as well, but it makes an `IOException` the root cause of the reported `IllegalArgumentException`. `FtpServerOutboundTests` and `SftpServerOutboundTests` assert that root cause, so the thrown exception is kept unchanged in type, message and cause instead. Signed-off-by: Jiwoo Lee <dlwldn30@naver.com>

…file GET Fixes: spring-projects#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 spring-projectsGH-11254. Attaching the `IOException` as cause moves the root cause of the reported `IllegalArgumentException` from itself to a `FileSystemException`. `FtpServerOutboundTests` and `SftpServerOutboundTests` pinned that incidental position via `hasRootCauseInstanceOf()`, which held only because the previous `Assert.isTrue()` left the chain empty. * Assert the exception type and message where they occur in the chain instead of at its root, so the tests no longer depend on nothing being wrapped The adjusted assertions pass against the unfixed gateway as well, so they do not encode this change. Signed-off-by: Jiwoo Lee <dlwldn30@naver.com>

…file GET Fixes: spring-projects#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 spring-projectsGH-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>

artembilan requested changes Aug 10, 2026

View reviewed changes

tknall force-pushed the GH-11253-file-mkdirs-race branch from 38841c2 to 35e958d Compare August 10, 2026 15:56

artembilan enabled auto-merge (squash) August 10, 2026 16:08

artembilan requested changes Aug 10, 2026

View reviewed changes

Fix import order violating checkstyle

053fee0

Signed-off-by: Thomas Knall <thomas.knall@prime-sign.com>

auto-merge was automatically disabled August 10, 2026 16:28
Head branch was pushed to by a user without write access

artembilan merged commit 98ef9fe into spring-projects:main Aug 10, 2026
3 checks passed

This was referenced Aug 17, 2026

AbstractRemoteFileOutboundGateway: concurrent GET into the same new local directory fails with "Failed to make local directory" #11277

Open

GH-11277: Fix local directory creation race in remote file GET #11278

Open

Navigation Menu

Uh oh!

GH-11253: Fix directory-creation race in `FileWritingMessageHandler` - #11254

GH-11253: Fix directory-creation race in `FileWritingMessageHandler`#11254
artembilan merged 2 commits into
spring-projects:mainfrom
tknall:GH-11253-file-mkdirs-race

tknall commented Aug 10, 2026

artembilan left a comment

artembilan Aug 10, 2026

tknall Aug 10, 2026

artembilan Aug 10, 2026

tknall Aug 10, 2026

artembilan Aug 10, 2026

tknall Aug 10, 2026

artembilan Aug 10, 2026

tknall Aug 10, 2026

artembilan Aug 10, 2026

tknall Aug 10, 2026

artembilan Aug 10, 2026

artembilan Aug 10, 2026

tknall Aug 10, 2026

artembilan left a comment

Uh oh!

Labels

2 participants

Uh oh!

Conversation

tknall commented Aug 10, 2026

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Labels

2 participants