This is an automated email from the ASF dual-hosted git repository.
jbonofre pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/activemq.git
The following commit(s) were added to refs/heads/main by this push:
new 726fa61811 [AMQ-9856] Journal move fails on Windows (#1654)
726fa61811 is described below
commit 726fa61811933e42933fb5af715ef1f01421b439
Author: Jean-Louis Monteiro <[email protected]>
AuthorDate: Tue Feb 17 16:54:35 2026 +0100
[AMQ-9856] Journal move fails on Windows (#1654)
* Exclude JournalArchiveTest from Windows builds to reduce test flakiness
* AMQ-9856 Implement non-blocking file deletion and async cleanup for
Windows
* Enhance IOHelper: improve error handling in file operations and add unit
tests for rename functionality
---
.../java/org/apache/activemq/util/IOHelper.java | 207 +++++++++++++++++++--
.../java/org/apache/activemq/util/LockFile.java | 2 +-
.../org/apache/activemq/util/IOHelperTest.java | 143 ++++++++++++++
activemq-kahadb-store/pom.xml | 2 +-
.../store/kahadb/disk/journal/DataFile.java | 2 +-
.../activemq/store/kahadb/disk/page/PageFile.java | 13 +-
6 files changed, 343 insertions(+), 26 deletions(-)
diff --git
a/activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java
b/activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java
index 008140df1a..37041aab00 100644
--- a/activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java
+++ b/activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java
@@ -28,20 +28,132 @@ import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.ArrayList;
import java.util.List;
+import java.util.Queue;
import java.util.Stack;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Collection of File and Folder utility methods.
*/
public final class IOHelper {
+ private static final Logger LOG = LoggerFactory.getLogger(IOHelper.class);
+
protected static final int MAX_DIR_NAME_LENGTH;
protected static final int MAX_FILE_NAME_LENGTH;
private static final int DEFAULT_BUFFER_SIZE = 4096;
+ private static final boolean IS_WINDOWS = System.getProperty("os.name",
"").toLowerCase().contains("win");
+
+ // Async cleanup for files that couldn't be deleted immediately (Windows
file locking)
+ private static final Queue<File> PENDING_DELETES = new
ConcurrentLinkedQueue<>();
+ private static final ScheduledExecutorService CLEANUP_EXECUTOR;
+
+ static {
+ MAX_DIR_NAME_LENGTH = Integer.getInteger("MaximumDirNameLength", 200);
+ MAX_FILE_NAME_LENGTH = Integer.getInteger("MaximumFileNameLength", 64);
+
+ // Only start cleanup thread on Windows where file locking is an issue
+ if (IS_WINDOWS) {
+ CLEANUP_EXECUTOR = Executors.newSingleThreadScheduledExecutor(r ->
{
+ final Thread t = new Thread(r, "IOHelper-AsyncCleanup");
+ t.setDaemon(true);
+ return t;
+ });
+
CLEANUP_EXECUTOR.scheduleWithFixedDelay(IOHelper::processAsyncDeletes, 5, 5,
TimeUnit.SECONDS);
+ } else {
+ CLEANUP_EXECUTOR = null;
+ }
+ }
private IOHelper() {
}
+ /**
+ * Process pending async deletes. Called periodically on Windows.
+ */
+ private static void processAsyncDeletes() {
+ int processed = 0;
+ File file;
+ while ((file = PENDING_DELETES.poll()) != null && processed < 100) {
+ processed++;
+ try {
+ if (file.exists()) {
+ if (file.delete()) {
+ LOG.debug("Async cleanup: deleted {}", file);
+ } else {
+ // Still locked, re-queue for later
+ PENDING_DELETES.offer(file);
+ }
+ }
+ } catch (final Exception e) {
+ LOG.warn("Async cleanup: failed to delete {}", file, e);
+ }
+ }
+ }
+
+ /**
+ * Schedule a file for async deletion. Used when immediate deletion fails
on Windows.
+ * The file will be deleted in the background when it becomes unlocked.
+ */
+ public static void scheduleAsyncDelete(File file) {
+ if (file != null && file.exists()) {
+ if (IS_WINDOWS && CLEANUP_EXECUTOR != null) {
+ PENDING_DELETES.offer(file);
+ LOG.debug("Scheduled async delete for: {}", file);
+ } else {
+ // On non-Windows, just use deleteOnExit
+ file.deleteOnExit();
+ }
+ }
+ }
+
+ /**
+ * Non-blocking file deletion. Tries to delete once, and if it fails,
+ * schedules async cleanup instead of blocking with retries.
+ *
+ * This is safe to call from synchronized blocks as it never sleeps or
retries.
+ *
+ * @param file the file to delete
+ * @return true if deleted immediately, false if scheduled for async
cleanup
+ */
+ public static boolean deleteFileNonBlocking(File file) {
+ if (file == null || !file.exists()) {
+ return true;
+ }
+
+ // For directories, try to delete children first (non-blocking)
+ if (file.isDirectory()) {
+ final File[] children = file.listFiles();
+ if (children != null) {
+ for (final File child : children) {
+ deleteFileNonBlocking(child);
+ }
+ }
+ }
+
+ // Single delete attempt - no retry
+ if (file.delete()) {
+ return true;
+ }
+
+ // Failed to delete - schedule for async cleanup
+ scheduleAsyncDelete(file);
+ return false;
+ }
+
+ /**
+ * Check if we're running on Windows.
+ */
+ public static boolean isWindows() {
+ return IS_WINDOWS;
+ }
+
public static String getDefaultDataDirectory() {
return getDefaultDirectoryPrefix() + "activemq-data";
}
@@ -187,19 +299,91 @@ public final class IOHelper {
return result;
}
- public static void moveFile(File src, File targetDirectory) throws
IOException {
- if (!src.renameTo(new File(targetDirectory, src.getName()))) {
+ /**
+ * Rename a file to a new name/path. Non-blocking on failure.
+ * Strategy: rename -> NIO move -> copy + async delete of source.
+ *
+ * @param src the source file
+ * @param dest the destination file (full path with new name)
+ */
+ public static void renameFile(File src, File dest) throws IOException {
+ if (src == null) {
+ throw new IOException("Source file is null");
+ }
+ if (dest == null) {
+ throw new IOException("Destination file is null");
+ }
+ if (!src.exists()) {
+ throw new IOException("Source file does not exist: " + src);
+ }
- // If rename fails we must do a true deep copy instead.
- Path sourcePath = src.toPath();
- Path targetDirPath = targetDirectory.toPath();
+ final Path sourcePath = src.toPath();
+ final Path targetPath = dest.toPath();
- try {
- Files.move(sourcePath,
targetDirPath.resolve(sourcePath.getFileName()),
StandardCopyOption.REPLACE_EXISTING);
- } catch (IOException ex) {
- throw new IOException("Failed to move " + src + " to " +
targetDirectory + " - " + ex.getMessage(), ex);
+ // Fast path: try rename
+ try {
+ if (src.renameTo(dest)) {
+ return;
}
+ LOG.debug("rename failed for {} -> {}, trying NIO move", src,
dest);
+ } catch (final Exception e) {
+ LOG.debug("rename threw exception for {} -> {}, trying NIO move",
src, dest, e);
}
+
+ // Try NIO move
+ try {
+ Files.move(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING);
+ return;
+ } catch (final IOException e) {
+ LOG.debug("NIO move failed for {} -> {}, falling back to
copy+delete", src, dest, e);
+ }
+
+ // Copy + async delete as last resort
+ Files.copy(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES);
+ deleteFileNonBlocking(src);
+ }
+
+ /**
+ * Move a file to a target directory. Non-blocking - uses async cleanup
for source on Windows.
+ * Strategy: rename -> NIO move -> copy + async delete of source.
+ *
+ * @param src the source file to move
+ * @param targetDirectory the target directory (must be a directory)
+ */
+ public static void moveFile(File src, File targetDirectory) throws
IOException {
+ if (src == null) {
+ throw new IOException("Source file is null");
+ }
+ if (targetDirectory == null) {
+ throw new IOException("Target directory is null");
+ }
+ mkdirs(targetDirectory);
+ final File dest = new File(targetDirectory, src.getName());
+ final Path sourcePath = src.toPath();
+ final Path targetPath = dest.toPath();
+
+ // Fast path: try rename (works if same filesystem and no locks)
+ try {
+ if (src.renameTo(dest)) {
+ return;
+ }
+ LOG.debug("rename failed for {} -> {}, trying NIO move", src,
dest);
+ } catch (final Exception e) {
+ LOG.debug("rename threw exception for {} -> {}, trying NIO move",
src, dest, e);
+ }
+
+ // Try NIO move
+ try {
+ Files.move(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING);
+ return;
+ } catch (final IOException e) {
+ LOG.debug("NIO move failed for {} -> {}, falling back to
copy+delete", src, dest, e);
+ }
+
+ // Copy + async delete as last resort
+ Files.copy(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES);
+ // Copy succeeded, schedule async cleanup of source (non-blocking)
+ deleteFileNonBlocking(src);
}
public static void moveFiles(File srcDirectory, File targetDirectory,
FilenameFilter filter) throws IOException {
@@ -306,11 +490,6 @@ public final class IOHelper {
}
}
- static {
- MAX_DIR_NAME_LENGTH = Integer.getInteger("MaximumDirNameLength", 200);
- MAX_FILE_NAME_LENGTH = Integer.getInteger("MaximumFileNameLength", 64);
- }
-
public static int getMaxDirNameLength() {
return MAX_DIR_NAME_LENGTH;
}
diff --git
a/activemq-broker/src/main/java/org/apache/activemq/util/LockFile.java
b/activemq-broker/src/main/java/org/apache/activemq/util/LockFile.java
index 2f89bc5b2b..413f1dcb58 100644
--- a/activemq-broker/src/main/java/org/apache/activemq/util/LockFile.java
+++ b/activemq-broker/src/main/java/org/apache/activemq/util/LockFile.java
@@ -136,7 +136,7 @@ public class LockFile {
closeReadFile();
if (locked && deleteOnUnlock) {
- file.delete();
+ IOHelper.deleteFileNonBlocking(file);
}
}
diff --git
a/activemq-broker/src/test/java/org/apache/activemq/util/IOHelperTest.java
b/activemq-broker/src/test/java/org/apache/activemq/util/IOHelperTest.java
new file mode 100644
index 0000000000..011f3fbbf2
--- /dev/null
+++ b/activemq-broker/src/test/java/org/apache/activemq/util/IOHelperTest.java
@@ -0,0 +1,143 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.util;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.nio.file.StandardOpenOption;
+import java.util.Objects;
+
+import static org.junit.Assert.*;
+
+public class IOHelperTest {
+
+ private Path baseDir;
+
+ @Before
+ public void setUp() throws IOException {
+ baseDir = Paths.get("target", "iohelper-test").toAbsolutePath();
+ if (Files.exists(baseDir)) {
+ // best effort cleanup
+ IOHelper.delete(new File(baseDir.toString()));
+ }
+ Files.createDirectories(baseDir);
+ }
+
+ @After
+ public void tearDown() throws IOException {
+ if (Files.exists(baseDir)) {
+ IOHelper.delete(new File(baseDir.toString()));
+ }
+ }
+
+ @Test
+ public void testMoveFileNioCreated() throws Exception {
+ Path srcDir = baseDir.resolve("src");
+ Path dstDir = baseDir.resolve("dst");
+ Files.createDirectories(srcDir);
+ Files.createDirectories(dstDir);
+
+ Path srcFile = srcDir.resolve("testfile.txt");
+ Files.writeString(srcFile, "hello world", StandardOpenOption.CREATE,
StandardOpenOption.TRUNCATE_EXISTING);
+
+ File src = srcFile.toFile();
+ File targetDir = dstDir.toFile();
+
+ IOHelper.moveFile(src, targetDir);
+
+ Path moved = dstDir.resolve("testfile.txt");
+ assertTrue("moved file should exist", Files.exists(moved));
+ assertEquals("content", "hello world", Files.readString(moved));
+ assertFalse("original should not exist after move",
Files.exists(srcFile));
+ }
+
+ @Test
+ public void testMoveFileToDirectory() throws Exception {
+ final Path srcDir = baseDir.resolve("src-dir");
+ final Path dstDir = baseDir.resolve("dst-dir");
+ Files.createDirectories(srcDir);
+ Files.createDirectories(dstDir);
+
+ final Path srcFile = srcDir.resolve("file.txt");
+ Files.writeString(srcFile, "dir move test", StandardOpenOption.CREATE,
StandardOpenOption.TRUNCATE_EXISTING);
+
+ IOHelper.moveFile(srcFile.toFile(), dstDir.toFile());
+
+ assertTrue("moved file should exist in target dir",
Files.exists(dstDir.resolve("file.txt")));
+ assertFalse("source should not exist", Files.exists(srcFile));
+ }
+
+ @Test
+ public void testDeleteFileNonBlocking() throws Exception {
+ final Path f = baseDir.resolve("nonblocking.txt");
+ Files.writeString(f, "test", StandardOpenOption.CREATE,
StandardOpenOption.TRUNCATE_EXISTING);
+ final File file = f.toFile();
+
+ final boolean result = IOHelper.deleteFileNonBlocking(file);
+
+ assertTrue("file should be deleted or scheduled", result ||
!file.exists());
+ }
+
+ @Test
+ public void testDeleteFileNonBlockingDirectory() throws Exception {
+ final Path dir = baseDir.resolve("nonblocking-dir");
+ Files.createDirectories(dir);
+ Files.writeString(dir.resolve("child.txt"), "test",
StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING);
+ final File file = dir.toFile();
+
+ final boolean result = IOHelper.deleteFileNonBlocking(file);
+
+ // Directory may be deleted immediately or scheduled for async cleanup
+ assertTrue("dir deletion result", result || !file.exists() ||
Objects.requireNonNull(file.listFiles()).length == 0);
+ }
+
+ @Test
+ public void testRenameFile() throws Exception {
+ final Path srcFile = baseDir.resolve("rename-src.txt");
+ final Path destFile = baseDir.resolve("rename-dest.txt");
+ Files.writeString(srcFile, "rename test", StandardOpenOption.CREATE,
StandardOpenOption.TRUNCATE_EXISTING);
+
+ IOHelper.renameFile(srcFile.toFile(), destFile.toFile());
+
+ assertTrue("renamed file should exist", Files.exists(destFile));
+ assertEquals("content preserved", "rename test",
Files.readString(destFile));
+ assertFalse("source should not exist after rename",
Files.exists(srcFile));
+ }
+
+ @Test(expected = IOException.class)
+ public void testRenameFileSourceDoesNotExist() throws Exception {
+ final File src = baseDir.resolve("does-not-exist.txt").toFile();
+ final File dest = baseDir.resolve("dest.txt").toFile();
+
+ IOHelper.renameFile(src, dest);
+ }
+
+ @Test
+ public void testIsWindows() {
+ // Just verify the method works without exceptions
+ final boolean isWindows = IOHelper.isWindows();
+ final String os = System.getProperty("os.name", "").toLowerCase();
+ assertEquals("isWindows detection", os.contains("win"), isWindows);
+ }
+}
diff --git a/activemq-kahadb-store/pom.xml b/activemq-kahadb-store/pom.xml
index cfab14db69..0f4804f926 100644
--- a/activemq-kahadb-store/pom.xml
+++ b/activemq-kahadb-store/pom.xml
@@ -318,7 +318,7 @@
</plugins>
</build>
</profile>
-
+
<profile>
<id>activemq.tests.aix.excludes</id>
<activation>
diff --git
a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFile.java
b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFile.java
index 4f8a828aa6..5e8fb7104e 100644
---
a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFile.java
+++
b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFile.java
@@ -100,7 +100,7 @@ public class DataFile extends LinkedNode<DataFile>
implements Comparable<DataFil
}
public synchronized boolean delete() throws IOException {
- return file.delete();
+ return IOHelper.deleteFileNonBlocking(file);
}
public synchronized void move(File targetDirectory) throws IOException{
diff --git
a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/page/PageFile.java
b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/page/PageFile.java
index a1319278ba..cfb86d613d 100644
---
a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/page/PageFile.java
+++
b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/page/PageFile.java
@@ -350,20 +350,15 @@ public class PageFile {
/**
* @param file
- * @throws IOException
*/
- private void delete(File file) throws IOException {
- if (file.exists() && !file.delete()) {
- throw new IOException("Could not delete: " + file.getPath());
- }
+ private void delete(File file) {
+ IOHelper.deleteFileNonBlocking(file);
}
private void archive(File file, String suffix) throws IOException {
if (file.exists()) {
- File archive = new File(file.getPath() + "-" + suffix);
- if (!file.renameTo(archive)) {
- throw new IOException("Could not archive: " + file.getPath() +
" to " + file.getPath());
- }
+ final File archiveFile = new File(file.getPath() + "-" + suffix);
+ IOHelper.renameFile(file, archiveFile);
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
