gnodet commented on code in PR #345:
URL:
https://github.com/apache/maven-clean-plugin/pull/345#discussion_r4110369769
##########
src/main/java/org/apache/maven/plugins/clean/Cleaner.java:
##########
@@ -294,7 +318,42 @@ public final void delete(@Nonnull Path basedir) throws
IOException {
logger.debug(fastDeleteError(e), e);
}
}
+ if (retryOnError) {
+ retryQueue = new ArrayList<>();
+ }
Files.walkFileTree(basedir, options, Integer.MAX_VALUE, this);
+ /*
+ * Batch retry: if any deletions failed during the walk, sleep once
and retry all of them.
+ * This is the same strategy used by
BackgroundCleaner.deleteInBackground() (see MCLEAN-102):
+ * a single sleep lets external processes (virus scanners, search
indexers) release file locks,
+ * without the stop-the-world System.gc() calls and per-file sleep
loops that were used before.
+ */
+ if (retryQueue != null && !retryQueue.isEmpty()) {
+ try {
+ Thread.sleep(BATCH_RETRY_DELAY_MS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ // Interrupted during the batch-retry sleep. Restore the flag
and stop retrying;
+ // any remaining paths in the queue are abandoned. The
caller's next blocking
+ // call (e.g. BlockingQueue.take() in BackgroundCleaner) will
see the flag.
+ retryQueue = null;
+ return;
+ }
+ for (Path path : retryQueue) {
+ try {
+ Files.deleteIfExists(path);
+ } catch (IOException e) {
+ if (logger.isWarnEnabled()) {
+ logger.warn("Failed to delete " + path + " after batch
retry", e);
Review Comment:
Fixed in 6722499. Batch retry now logs 'Deleted file/directory' on
successful retry using the new RetryEntry record (carries the directory flag).
Walk-phase exception is also attached to the warning via the exception map.
##########
src/main/java/org/apache/maven/plugins/clean/Cleaner.java:
##########
@@ -355,7 +414,7 @@ public FileVisitResult preVisitDirectory(Path dir,
BasicFileAttributes attrs) th
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
throws IOException {
if (fileMatcher.matches(file) && tryDelete(file)) {
- if (listDeletedFiles) {
+ if (listDeletedFiles && !pendingRetry) {
Review Comment:
Fixed in 6722499. The retry loop now calls logDelete-style logging on
success: 'Deleted file/directory <path>' at the appropriate level (info if
verbose, debug otherwise).
##########
src/test/java/org/apache/maven/plugins/clean/CleanerTest.java:
##########
@@ -134,4 +136,85 @@ void
deleteDoesNotLogAnythingWhenNoPermissionAndWarnDisabled(@TempDir Path tempD
assertDoesNotThrow(() -> cleaner.delete(basedir));
verify(log, never()).warn(any(CharSequence.class),
any(Throwable.class));
}
+
+ /**
+ * Verifies the batch retry strategy: a file that fails during the tree
walk but becomes
+ * deletable before the batch retry completes is deleted without any error
being reported.
+ *
+ * <p>This test simulates the Windows scenario where a file is temporarily
locked by an
+ * external process (virus scanner, search indexer): the directory is made
non-writable during
+ * the walk, then a background thread restores write permission during the
+ * {@value Cleaner#BATCH_RETRY_DELAY_MS}ms batch-retry sleep, allowing the
retry to succeed.
+ * No {@code System.gc()} is called between attempts — that was the
anti-pattern removed by
+ * this fix (see issue #281).</p>
+ */
+ @Test
+ @DisabledOnOs(OS.WINDOWS)
+ void batchRetrySucceedsWhenFileBecomesAvailableDuringDelay(@TempDir Path
tempDir) throws Exception {
+ when(log.isWarnEnabled()).thenReturn(true);
+ final Path basedir =
createDirectory(tempDir.resolve("target")).toRealPath();
+ final Path file = createFile(basedir.resolve("file"));
+
+ // Make the directory non-writable so the first deletion attempt on
the file fails.
+ final Set<PosixFilePermission> noWrite =
PosixFilePermissions.fromString("r-xr-xr-x");
+ final Set<PosixFilePermission> writable =
PosixFilePermissions.fromString("rwxrwxr-x");
+ setPosixFilePermissions(basedir, noWrite);
+
+ // Schedule a background thread to restore write permission during the
batch-retry sleep.
+ // The batch sleep is BATCH_RETRY_DELAY_MS (250ms); we restore after
50ms to give plenty of margin.
+ final CountDownLatch walkStarted = new CountDownLatch(1);
+ Thread restorer = new Thread(() -> {
+ try {
+ walkStarted.await(5, TimeUnit.SECONDS);
+ Thread.sleep(50);
+ setPosixFilePermissions(basedir, writable);
+ } catch (Exception ignored) {
+ }
+ });
+ restorer.setDaemon(true);
+ restorer.start();
+
+ // Signal the restorer thread immediately before starting the delete.
+ walkStarted.countDown();
Review Comment:
Fixed in 6722499. Added elapsed-time assertion: `assertTrue(elapsed >= 200,
...)` — the test now fails if the batch retry sleep never runs. Also tightened
the warning-count test to assert exactly 1 warning (the leaf file), verifying
the cascade fix.
##########
src/main/java/org/apache/maven/plugins/clean/Cleaner.java:
##########
@@ -476,68 +535,77 @@ private static Path setWritable(Path file, int
currentDepth) throws IOException
}
/**
- * Deletes the specified file or directory.
+ * Attempts to delete the specified file or directory (one attempt only
during the walk phase).
* If the path denotes a symlink, only the link is removed. Its target is
left untouched.
* This method returns {@code true} if the file has been deleted, or
{@code false} if the
* file does not exist or if an {@link IOException} occurred but {@link
#failOnError} is
* {@code false}.
*
+ * <h4>Retry strategy</h4>
+ * If deletion fails with an {@link AccessDeniedException} and {@code
force} is enabled,
+ * this method first tries to make the file writable and retries
<em>immediately</em>
+ * (this is a permissions fix, not a timing retry).
+ * For all other transient failures (e.g. Windows file locks held by virus
scanners),
+ * the path is added to {@link #retryQueue} and the batch retry in {@link
#delete(Path)}
+ * will retry it after a single {@value #BATCH_RETRY_DELAY_MS}ms sleep —
without calling
+ * {@code System.gc()} or sleeping per file.
+ *
* <h4>Auxiliary information as side-effect</h4>
* This method sets the {@link #reallyDeletedLastFile} flag to whether
this method really deleted the file.
* If that flag is {@code false} after this method returned {@code true},
then the file has been deleted by
- * some concurrent process before this method tried to deleted the file.
+ * some concurrent process before this method tried to delete the file.
* That flag is used for logging purpose only.
*
* @param file the file/directory to delete, must not be {@code null}
- * @return whether the file has been deleted or did not existed anymore by
the time this method is invoked
+ * @return whether the file has been deleted or did not exist anymore by
the time this method is invoked
* @throws IOException if a file/directory could not be deleted and {@code
failOnError} is {@code true}
*/
- @SuppressWarnings("SleepWhileInLoop")
private boolean tryDelete(final Path file) throws IOException {
+ pendingRetry = false;
try {
reallyDeletedLastFile = Files.deleteIfExists(file);
return true;
} catch (IOException failure) {
- boolean tryWritable = force && failure instanceof
AccessDeniedException;
- if (tryWritable || retryOnError) {
- final Set<Path> madeWritable; // Safety against never-ending
loops.
- if (force) {
- madeWritable = new HashSet<>();
- madeWritable.add(null); // For having `add(null)` to
return `false`.
- } else {
- madeWritable = null;
- }
- final var alreadyReported = new HashMap<Class<?>,
Set<String>>(); // For avoiding repetition.
- isNewError(alreadyReported, failure);
- int delayIndex = 0;
- while (delayIndex < RETRY_DELAYS.length) {
- if (tryWritable) {
- tryWritable = madeWritable.add(setWritable(file,
currentDepth));
- // `true` if we successfully changed permission, in
which case we will skip the delay.
- }
- if (!tryWritable) {
- if (ON_WINDOWS) {
- // Try to release any locks held by non-closed
files.
- System.gc();
- }
- try {
- Thread.sleep(RETRY_DELAYS[delayIndex++]);
- } catch (InterruptedException e) {
- failure.addSuppressed(e);
- throw failure;
- }
- }
+ /*
+ * If force=true and the failure is an AccessDeniedException, try
to make the file
+ * writable and retry immediately. This is a permissions fix (not
a timing retry)
+ * and is safe to do inline without a sleep.
+ */
+ if (force && failure instanceof AccessDeniedException) {
+ final Set<Path> madeWritable = new HashSet<>();
+ madeWritable.add(null); // sentinel so add(null) returns false
+ while (madeWritable.add(setWritable(file, currentDepth))) {
try {
reallyDeletedLastFile = Files.deleteIfExists(file);
return true;
} catch (IOException again) {
- tryWritable = force && failure instanceof
AccessDeniedException;
- if (isNewError(alreadyReported, again)) {
+ if (!(again instanceof AccessDeniedException)) {
failure.addSuppressed(again);
+ failure = again;
+ break;
}
+ failure.addSuppressed(again);
+ failure = again;
Review Comment:
Fixed in 6722499. Chaining is now `again.addSuppressed(failure); failure =
again;` — the full history is reachable from the current `failure`. Also
collapsed the two branches as you suggested.
##########
src/main/java/org/apache/maven/plugins/clean/Cleaner.java:
##########
@@ -294,7 +318,42 @@ public final void delete(@Nonnull Path basedir) throws
IOException {
logger.debug(fastDeleteError(e), e);
}
}
+ if (retryOnError) {
+ retryQueue = new ArrayList<>();
+ }
Files.walkFileTree(basedir, options, Integer.MAX_VALUE, this);
+ /*
+ * Batch retry: if any deletions failed during the walk, sleep once
and retry all of them.
+ * This is the same strategy used by
BackgroundCleaner.deleteInBackground() (see MCLEAN-102):
+ * a single sleep lets external processes (virus scanners, search
indexers) release file locks,
+ * without the stop-the-world System.gc() calls and per-file sleep
loops that were used before.
+ */
+ if (retryQueue != null && !retryQueue.isEmpty()) {
+ try {
+ Thread.sleep(BATCH_RETRY_DELAY_MS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ // Interrupted during the batch-retry sleep. Restore the flag
and stop retrying;
+ // any remaining paths in the queue are abandoned. The
caller's next blocking
+ // call (e.g. BlockingQueue.take() in BackgroundCleaner) will
see the flag.
Review Comment:
Fixed in 6722499. Removed the incorrect BlockingQueue reference. The
interrupt path now logs `logger.warn("Interrupted during batch-retry sleep; N
path(s) were not retried and may remain on disk.")` so the user has a trace.
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -255,16 +255,22 @@ boolean fastDelete(Path baseDir) throws IOException {
}
/**
- * Deletes the given directory without logging messages and without
throwing {@link IOException}.
+ * Deletes the given directory without throwing {@link IOException}.
* The exceptions are stored for reporting after the end of the session.
*
+ * <p>Uses a copy of this cleaner ({@link Cleaner#Cleaner(Cleaner)}) which
shares
+ * the {@code retryOnError} configuration. The batch-retry strategy in
+ * {@link Cleaner#delete(Path)} ({@value Cleaner#BATCH_RETRY_DELAY_MS}ms
sleep
+ * after the full tree walk) applies automatically — no per-file {@code
System.gc()}
+ * is called.</p>
+ *
* <h4>Thread safety</h4>
* Contrarily to most other methods in {@code BackgroundCleaner}, this
method is
* thread-safe because it uses a copy of this cleaner for walking in the
file tree.
*/
private void deleteSilently(final Path dir) {
try {
- Files.walkFileTree(dir, Set.of(), Integer.MAX_VALUE, new
Cleaner(this));
+ new Cleaner(this).delete(dir);
Review Comment:
Acknowledged in 6722499. Expanded the deleteSilently Javadoc to explain why
the info log is acceptable for .clean/ temp directories. Re: #347 conflict —
agreed this needs coordination. If #347 lands first, the BackgroundCleaner
change here disappears. Happy to rebase on top of it.
##########
src/main/java/org/apache/maven/plugins/clean/Cleaner.java:
##########
@@ -145,17 +152,34 @@ class Cleaner implements FileVisitor<Path> {
private final boolean retryOnError;
/**
- * The delays (in milliseconds) if {@link #retryOnError} is {@code true}.
- * The length of this array is the maximal number of new attempts.
+ * Delay in milliseconds for the single batch retry after a full tree walk.
+ * {@link BackgroundCleaner} reuses this constant via {@link
#delete(Path)} (called from
+ * {@code deleteSilently}): both the foreground and background clean paths
apply a single sleep
+ * before retrying all failures, without any per-file {@code System.gc()}
call.
+ *
+ * @see #delete(Path)
*/
- private static final int[] RETRY_DELAYS = new int[] {50, 250, 750};
+ static final int BATCH_RETRY_DELAY_MS = 250;
Review Comment:
Fixed in 6722499. Made BATCH_RETRY_DELAY_MS `private static final` and
replaced all `{@value}` references with plain prose '250 ms'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
