slawekjaranowski commented on code in PR #345:
URL:
https://github.com/apache/maven-clean-plugin/pull/345#discussion_r4107264889
##########
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:
Two things here.
- This is where the ancestor warnings pile up; see the comment on the
tentative `return true`.
- The reported exception is the fresh one from this `Files.deleteIfExists`,
so the original failure from the walk phase — including its
`AccessDeniedException` message — is not attached. Queueing the walk-phase
exception together with the path and chaining the two would make the warning
diagnosable.
Also, with `failOnError=true` this `throw e` leaves the loop, so the
remaining queued paths are never attempted and never reported. Master aborted
mid-walk instead, so this is a change either way, but the silently skipped tail
is worth a thought.
##########
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:
Suppressing the log here is right, but nothing logs the deletion when the
batch retry later succeeds, so `verbose=true` loses the message entirely.
Measured against master:
| | with `verbose=true` |
|---|---|
| master | `Deleting "…/target".`, `Deleted file …/late.txt`, `Deleted
directory …/target` |
| this PR | `Deleting "…/target".` only |
The retry loop would have to call `logDelete` on success. That needs the
`BasicFileAttributes` — or at least the directory-vs-file bit — to travel with
the path, which is another reason to queue a small record rather than a bare
`Path`.
##########
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;
}
}
}
+ /*
+ * The deletion failed. If retryOnError=true, add the path to the
batch retry queue
+ * rather than sleeping here. The batch retry in delete(Path) will
sleep once after
+ * the full tree walk and retry all failures together (see
MCLEAN-102 / #281).
+ * No System.gc() is called — that is an unreliable,
stop-the-world hint.
+ *
+ * Return true (tentatively successful) so that the caller does
not mark the parent
+ * directory as non-empty: if the batch retry succeeds for this
path, the parent
+ * directory will also be retried and deleted. If the batch retry
also fails, the
+ * directory deletion will fail with DirectoryNotEmptyException
and be reported.
+ */
+ if (retryQueue != null) {
+ retryQueue.add(file);
+ reallyDeletedLastFile = false;
+ pendingRetry = true;
+ return true; // tentatively: we expect the retry to succeed
Review Comment:
This is the root of the warning cascade in the summary. Returning `true`
makes `visitFile` and `postVisitDirectory` skip their `else` branch, so
`nonEmptyDirectoryLevels` is never set for this level; every ancestor directory
is then attempted, fails with `DirectoryNotEmptyException`, and is queued as
well. For `target/a/b/c/locked.txt` that is 5 queued paths and 5 warnings where
master logged 1, and it scales with depth.
(`failureCount` inflates the same way, but that field is never read anywhere
— pre-existing dead code, not something this PR has to deal with.)
The tentative `true` is what lets the parents succeed after a successful
retry, so I would keep it and make the *reporting* root-cause aware instead: in
the batch retry, skip the warning for a `DirectoryNotEmptyException` on a
directory that still has a failing descendant in the same queue, and report
only the leaf cause. That gives one warning per real problem.
##########
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:
I think the chaining is the wrong way round, despite the "exception
chaining" fix in fd1ed42. `failure.addSuppressed(again)` attaches `again` to
the object that the very next statement stops referencing, so after a second
iteration the original `AccessDeniedException` is unreachable from `failure`
and only the last exception is reported.
`again.addSuppressed(failure); failure = again;` would keep the history — or
keep the original in `failure` and only ever add to it.
Separately, both branches of the `if` above run the same two statements and
differ only by the `break`, so it could collapse to:
```java
failure.addSuppressed(again); // or the corrected order
failure = again;
if (!(again instanceof AccessDeniedException)) {
break;
}
```
I could not reproduce the loss on POSIX — once `setWritable` succeeds the
deletion succeeds, so the loop does not iterate twice — so this one is from
reading the code, not from a measurement.
##########
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:
`walkStarted` is counted down here, before `cleaner.delete()` is called
below, so the latch does not mark the start of the walk and the restorer's 50
ms timer starts before the walk begins. What actually keeps the test honest is
the 50 ms versus 250 ms margin.
More importantly, the assertions cannot detect the batch retry at all. I ran
this test with the `setPosixFilePermissions(basedir, noWrite)` line removed —
nothing fails, so the retry never runs — and `verify(log, never()).warn(...)`
plus both `assertFalse(exists(...))` still pass. To pin the behaviour the test
needs a positive signal that the retry ran: assert that the walk-phase attempt
really failed, inject the sleep, or at minimum assert elapsed time is at least
`BATCH_RETRY_DELAY_MS`.
##########
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:
The Javadoc says this is package-visible so that
`BackgroundCleaner.deleteSilently`'s Javadoc can reference it through
`{@value}`. Widening visibility only to satisfy a doc link is thin — `private`
plus a plain "250 ms" in the other class's prose would read the same. And if
#347 lands, `BackgroundCleaner` gets its own constant of the same name, so the
two need reconciling regardless.
##########
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:
There is no `BlockingQueue` in `BackgroundCleaner`; it uses an
`ExecutorService`, and `grep -rn BlockingQueue src/` matches only this comment.
Worth rewording so it does not send the next reader looking for code that is
not there.
On the behaviour itself: this path abandons the queue with no warning and no
report, so an interrupt during the batch sleep leaves files behind while
`delete()` returns normally to `CleanMojo`, which treats that as success. A
`logger.warn` before returning would at least leave 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:
Two concerns.
- `Cleaner.delete(Path)` logs `logger.info("Deleting \"" + basedir + "\".")`
unconditionally, so every staged directory now produces an info line from the
background thread at session end — paths like `…/target/.clean/module-12345`
that mean nothing to the user. This method's contract was "without logging
messages", and the PR edits that sentence out of the Javadoc rather than
addressing it.
- #347 removes `deleteSilently` and the `Cleaner(Cleaner)` copy-constructor
usage altogether and adds its own `BATCH_RETRY_DELAY_MS` to
`BackgroundCleaner`; `git merge-tree` confirms the two branches conflict in
both files. If #347 lands first this hunk disappears, so it may be cleanest to
drop the `BackgroundCleaner` change here and rebase the `Cleaner.java` part on
top of #347.
--
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]
