[
https://issues.apache.org/jira/browse/AMQ-9856?focusedWorklogId=1004876&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1004876
]
ASF GitHub Bot logged work on AMQ-9856:
---------------------------------------
Author: ASF GitHub Bot
Created on: 12/Feb/26 14:59
Start Date: 12/Feb/26 14:59
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1654:
URL: https://github.com/apache/activemq/pull/1654#discussion_r2799381228
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -187,19 +295,89 @@ public static boolean deleteChildren(File parent) {
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()) {
+ return;
+ }
+
+ final Path sourcePath = src.toPath();
+ final Path targetPath = dest.toPath();
+
+ // Fast path: try rename
+ try {
+ if (src.renameTo(dest)) {
+ return;
+ }
+ } catch (Exception e) {
+ // Fall through to NIO move
Review Comment:
The renameFile method catches all Exceptions on line 324, including checked
exceptions and RuntimeExceptions. This overly broad catch could mask serious
errors. Consider catching only specific exceptions or at minimum, logging the
exception before falling through to the NIO move fallback.
```suggestion
} catch (SecurityException e) {
// Fall through to NIO move on security manager failure
```
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -187,19 +295,89 @@ public static boolean deleteChildren(File parent) {
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()) {
+ return;
Review Comment:
The renameFile method silently falls through to copy+delete when the source
file doesn't exist (line 313 returns early), but this means a non-existent
source is treated as success. While this may be intentional for idempotency, it
differs from the original behavior which would have thrown IOException.
Consider whether this change is desired or if an exception should still be
thrown for non-existent sources.
```suggestion
throw new IOException("Source file does not exist: " + src);
```
##########
activemq-broker/src/test/java/org/apache/activemq/util/IOHelperTest.java:
##########
@@ -0,0 +1,122 @@
+/**
+ * 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);
Review Comment:
The test assertion on line 112 uses Objects.requireNonNull without
null-checking file.listFiles() first. While this may be acceptable for testing,
file.listFiles() can return null if the file is not a directory or if an I/O
error occurs. This could lead to a NullPointerException instead of a meaningful
test failure. Consider checking for null explicitly before calling
Objects.requireNonNull.
```suggestion
final File[] children = file.listFiles();
assertNotNull("listFiles should not return null for directory " +
file, children);
// Directory may be deleted immediately or scheduled for async
cleanup
assertTrue("dir deletion result", result || !file.exists() ||
children.length == 0);
```
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -187,19 +295,89 @@ public static boolean deleteChildren(File parent) {
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()) {
+ return;
+ }
+
+ final Path sourcePath = src.toPath();
+ final Path targetPath = dest.toPath();
+
+ // Fast path: try rename
+ try {
+ if (src.renameTo(dest)) {
+ return;
+ }
+ } catch (Exception e) {
+ // Fall through to NIO move
+ }
+
+ // Try NIO move
+ try {
+ Files.move(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING);
+ return;
+ } catch (IOException e) {
+ // Fall through to copy+delete
+ }
+
+ // Copy + async delete as last resort
+ Files.copy(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES);
+ deleteFileNonBlocking(src);
+ }
- // If rename fails we must do a true deep copy instead.
- Path sourcePath = src.toPath();
- Path targetDirPath = targetDirectory.toPath();
+ /**
+ * 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);
Review Comment:
The moveFile method calls mkdirs on line 355 without checking the result. If
the directory creation fails, the subsequent move operations will also fail but
with potentially confusing error messages. Consider checking the return value
of mkdirs and throwing a more descriptive IOException if directory creation
fails.
```suggestion
if (!targetDirectory.exists() && !mkdirs(targetDirectory)) {
throw new IOException("Could not create target directory: " +
targetDirectory);
}
```
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -187,19 +295,89 @@ public static boolean deleteChildren(File parent) {
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()) {
+ return;
+ }
+
+ final Path sourcePath = src.toPath();
+ final Path targetPath = dest.toPath();
+
+ // Fast path: try rename
+ try {
+ if (src.renameTo(dest)) {
+ return;
+ }
+ } catch (Exception e) {
+ // Fall through to NIO move
+ }
+
+ // Try NIO move
+ try {
+ Files.move(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING);
+ return;
+ } catch (IOException e) {
+ // Fall through to copy+delete
+ }
+
+ // Copy + async delete as last resort
+ Files.copy(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES);
+ deleteFileNonBlocking(src);
+ }
- // If rename fails we must do a true deep copy instead.
- Path sourcePath = src.toPath();
- Path targetDirPath = targetDirectory.toPath();
+ /**
+ * 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();
- 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 (works if same filesystem and no locks)
+ try {
+ if (src.renameTo(dest)) {
+ return;
}
+ } catch (Exception e) {
+ // Fall through to NIO move
}
+
+ // Try NIO move
+ try {
+ Files.move(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING);
+ return;
+ } catch (IOException e) {
+ // Fall through to copy+delete
+ }
Review Comment:
The moveFile method also silently catches and ignores IOException during NIO
move attempts (lines 370-375), falling back to copy+delete. However, there's no
logging of these failures. This makes it difficult to diagnose issues in
production when move operations fail. Consider adding debug or trace logging
for these fallback scenarios.
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -28,20 +28,128 @@
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);
Review Comment:
The daemon thread created for async cleanup (line 65) runs with the security
context of the class loader that first loads IOHelper. In environments with
SecurityManager enabled, this thread may not have permissions to delete files
in all scenarios, especially if files are added to the queue from different
security contexts. Consider documenting this limitation or adding appropriate
security checks.
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -28,20 +28,128 @@
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++;
+ if (file.exists()) {
+ if (file.delete()) {
+ LOG.debug("Async cleanup: deleted {}", file);
+ } else {
+ // Still locked, re-queue for later
+ PENDING_DELETES.offer(file);
+ }
+ }
+ }
+ }
+
+ /**
+ * 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);
Review Comment:
Method scheduleAsyncDelete ignores exceptional return value of
Queue<File>.offer.
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -28,20 +28,128 @@
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++;
+ if (file.exists()) {
+ if (file.delete()) {
+ LOG.debug("Async cleanup: deleted {}", file);
+ } else {
+ // Still locked, re-queue for later
+ PENDING_DELETES.offer(file);
+ }
+ }
+ }
+ }
+
+ /**
+ * 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();
+ }
+ }
Review Comment:
Potential race condition in scheduleAsyncDelete: the method checks
file.exists() at line 101 and then adds to PENDING_DELETES at line 103, but the
file could be deleted by another thread between these operations. While this
won't cause a critical failure (the async cleanup will simply skip non-existent
files), it could lead to unnecessary queue entries. Consider moving the exists
check into processAsyncDeletes or removing it entirely since the async
processor already checks existence.
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -28,20 +28,128 @@
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++;
+ if (file.exists()) {
+ if (file.delete()) {
+ LOG.debug("Async cleanup: deleted {}", file);
+ } else {
+ // Still locked, re-queue for later
+ PENDING_DELETES.offer(file);
Review Comment:
Method processAsyncDeletes ignores exceptional return value of
Queue<File>.offer.
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -28,20 +28,128 @@
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;
+ }
+ }
Review Comment:
The CLEANUP_EXECUTOR thread is never shut down. This daemon thread will
continue running indefinitely and may prevent proper JVM shutdown in some
cases. Consider adding a shutdown hook or a public shutdown method to properly
terminate the executor service, especially for unit testing scenarios where the
IOHelper class may be loaded and unloaded multiple times.
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -28,20 +28,128 @@
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++;
+ if (file.exists()) {
+ if (file.delete()) {
+ LOG.debug("Async cleanup: deleted {}", file);
+ } else {
+ // Still locked, re-queue for later
+ PENDING_DELETES.offer(file);
Review Comment:
The PENDING_DELETES queue has no size limit and could grow unbounded if
files remain locked for extended periods. The processAsyncDeletes method only
processes up to 100 files per run and re-queues files that are still locked. In
scenarios with persistent file locks, this could lead to memory issues.
Consider adding a maximum queue size limit or implementing a strategy to
eventually give up on files that remain locked for too long.
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -187,19 +295,89 @@ public static boolean deleteChildren(File parent) {
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()) {
+ return;
+ }
+
+ final Path sourcePath = src.toPath();
+ final Path targetPath = dest.toPath();
+
+ // Fast path: try rename
+ try {
+ if (src.renameTo(dest)) {
+ return;
+ }
+ } catch (Exception e) {
+ // Fall through to NIO move
+ }
+
+ // Try NIO move
+ try {
+ Files.move(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING);
+ return;
+ } catch (IOException e) {
+ // Fall through to copy+delete
+ }
Review Comment:
The renameFile and moveFile methods silently catch and ignore IOException
during NIO move attempts (lines 332-334 and 373-375), falling back to
copy+delete. However, there's no logging of these failures. This makes it
difficult to diagnose issues in production when rename operations fail.
Consider adding debug or trace logging for these fallback scenarios.
##########
activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/page/PageFile.java:
##########
@@ -350,20 +350,15 @@ public void archive() throws IOException {
/**
* @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);
}
Review Comment:
The delete method's signature no longer includes 'throws IOException' in its
Javadoc, but the behavior has changed significantly. Previously, delete would
throw an IOException if the file deletion failed. Now with
deleteFileNonBlocking, failures are silently scheduled for async cleanup. This
is a breaking API change that may affect error handling in calling code. The
Javadoc at line 329 should be updated to reflect that IOException is no longer
thrown for deletion failures.
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -187,19 +295,89 @@ public static boolean deleteChildren(File parent) {
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()) {
+ return;
+ }
+
+ final Path sourcePath = src.toPath();
+ final Path targetPath = dest.toPath();
+
+ // Fast path: try rename
+ try {
+ if (src.renameTo(dest)) {
+ return;
+ }
+ } catch (Exception e) {
+ // Fall through to NIO move
+ }
+
+ // Try NIO move
+ try {
+ Files.move(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING);
+ return;
+ } catch (IOException e) {
+ // Fall through to copy+delete
+ }
+
+ // Copy + async delete as last resort
+ Files.copy(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES);
+ deleteFileNonBlocking(src);
+ }
- // If rename fails we must do a true deep copy instead.
- Path sourcePath = src.toPath();
- Path targetDirPath = targetDirectory.toPath();
+ /**
+ * 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();
- 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 (works if same filesystem and no locks)
+ try {
+ if (src.renameTo(dest)) {
+ return;
}
+ } catch (Exception e) {
+ // Fall through to NIO move
}
+
+ // Try NIO move
+ try {
+ Files.move(sourcePath, targetPath,
StandardCopyOption.REPLACE_EXISTING);
+ return;
+ } catch (IOException e) {
+ // Fall through to copy+delete
+ }
+
+ // 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);
}
Review Comment:
In the renameFile and moveFile methods, if Files.copy succeeds but
deleteFileNonBlocking fails to immediately delete the source file, the source
file is scheduled for async cleanup. This could lead to a period where both the
source and destination files exist simultaneously. For applications expecting
atomic move semantics, this could cause issues. Consider documenting this
behavior change and its implications, especially for journal file management.
##########
activemq-broker/src/test/java/org/apache/activemq/util/IOHelperTest.java:
##########
@@ -0,0 +1,122 @@
+/**
+ * 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());
+ }
Review Comment:
The test assertions for deleteFileNonBlocking allow either immediate
deletion (result = true) or deferred deletion (result = false), but they don't
verify that deferred deletions actually complete. This is particularly
important for Windows testing where the async cleanup mechanism is exercised.
Consider adding a follow-up assertion that waits briefly and checks if
scheduled files are eventually deleted, or at least verifies they were added to
the pending queue.
##########
activemq-broker/src/test/java/org/apache/activemq/util/IOHelperTest.java:
##########
@@ -0,0 +1,122 @@
+/**
+ * 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 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);
+ }
+}
Review Comment:
The test suite is missing test coverage for the renameFile method, which is
a new public API method introduced in this PR. Consider adding tests that
verify the rename operation, including scenarios where rename fails and falls
back to copy+delete, and where the source file doesn't exist.
##########
activemq-broker/src/main/java/org/apache/activemq/util/IOHelper.java:
##########
@@ -28,20 +28,128 @@
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++;
+ if (file.exists()) {
+ if (file.delete()) {
+ LOG.debug("Async cleanup: deleted {}", file);
+ } else {
+ // Still locked, re-queue for later
+ PENDING_DELETES.offer(file);
+ }
Review Comment:
The processAsyncDeletes method lacks exception handling. If file.exists() or
file.delete() throw any unchecked exceptions (like SecurityException), the
scheduled task could fail and stop running entirely. Consider wrapping the loop
body in a try-catch block to ensure the async cleanup continues functioning
even if individual file operations fail.
```suggestion
try {
processed++;
if (file.exists()) {
if (file.delete()) {
LOG.debug("Async cleanup: deleted {}", file);
} else {
// Still locked, re-queue for later
PENDING_DELETES.offer(file);
}
}
} catch (Throwable t) {
LOG.warn("Async cleanup: failed to process pending delete
for {}", file, t);
```
Issue Time Tracking
-------------------
Worklog Id: (was: 1004876)
Time Spent: 20m (was: 10m)
> KahaDB Journal move fails on Windows
> ------------------------------------
>
> Key: AMQ-9856
> URL: https://issues.apache.org/jira/browse/AMQ-9856
> Project: ActiveMQ
> Issue Type: Bug
> Components: KahaDB
> Reporter: Jean-Louis Monteiro
> Assignee: Jean-Louis Monteiro
> Priority: Major
> Fix For: 6.3.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> On Windows, files cannot be renamed/moved while open by another process. The
> simplest and most appropriate fix is to skip this test on Windows where file
> locking makes it inherently unreliable.
> {code:java}
> java.io.IOException: Failed to move
> target\activemq-data\localhost\KahaDB\db-6.log to
> D:\a\activemq\activemq\activemq-kahadb-store\target\activemq-data\localhost\KahaDB\data-archive
> - target\activemq-data\localhost\KahaDB\db-6.log ->
> D:\a\activemq\activemq\activemq-kahadb-store\target\activemq-data\localhost\KahaDB\data-archive\db-6.log:
> The process cannot access the file because it is being used by another
> process
> at org.apache.activemq.util.IOHelper.moveFile(IOHelper.java:200)
> at
> org.apache.activemq.store.kahadb.disk.journal.DataFile.move(DataFile.java:107)
> at
> org.apache.activemq.store.kahadb.disk.journal.Journal.forceRemoveDataFile(Journal.java:833)
> at
> org.apache.activemq.store.kahadb.disk.journal.Journal.removeDataFiles(Journal.java:812)
> at
> org.apache.activemq.store.kahadb.MessageDatabase.checkpointUpdate(MessageDatabase.java:1701)
> at
> org.apache.activemq.store.kahadb.MessageDatabase.checkpointCleanup(MessageDatabase.java:1035)
> at
> org.apache.activemq.store.kahadb.KahaDBStore.checkpoint(KahaDBStore.java:1481)
> {code}
>
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
