gnodet-bot commented on code in PR #345:
URL:
https://github.com/apache/maven-clean-plugin/pull/345#discussion_r4088044468
##########
src/main/java/org/apache/maven/plugins/clean/Cleaner.java:
##########
@@ -294,7 +318,37 @@ 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();
+ }
Review Comment:
⚠️ **Interrupt contract broken — the retry loop should stop on interrupt.**
After `Thread.currentThread().interrupt()`, the code falls through into the
`for (Path path : retryQueue)` loop and continues calling
`Files.deleteIfExists()` for every remaining queued path. The interrupt flag is
preserved, so the caller's _next_ blocking call will see it — but the current
thread spends up to `N * syscall-latency` completing all retries before
honoring the signal.
The old code honored the interrupt immediately: `failure.addSuppressed(e);
throw failure;` — the walk stopped at the first interrupted sleep. The new
behavior is a silent regression for `BackgroundCleaner`: when Maven is shutting
down and the background thread is interrupted (e.g. during
`executor.shutdown()` → `awaitTermination(2s)`), the thread will keep retrying
all queued paths before it can exit, potentially causing the 2-second shutdown
window to be consumed by blocked filesystem calls.
Fix: break out of the retry loop after restoring the interrupt flag.
```suggestion
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;
}
```
##########
src/main/java/org/apache/maven/plugins/clean/Cleaner.java:
##########
@@ -476,68 +530,75 @@ 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)) {
- failure.addSuppressed(again);
+ if (!(again instanceof AccessDeniedException)) {
+ failure = again;
+ break;
}
+ failure = again;
Review Comment:
🔍 **Diagnostic regression: original `AccessDeniedException` is silently
dropped.**
When `setWritable()` succeeds but the subsequent `Files.deleteIfExists()`
throws a non-`AccessDeniedException` (line 577-579), `failure` is replaced by
`again` and the original `AccessDeniedException` is lost. In the old code this
was `failure.addSuppressed(again)`, which preserved the full failure chain.
The same replacement happens at line 581 when `again` _is_ an
`AccessDeniedException` — that one is less problematic since it's the same
type, but the loop iterates `setWritable` again with the new `failure`, losing
the history of previous attempts.
Fix: chain rather than replace:
```suggestion
failure.addSuppressed(again);
failure = again;
```
(Preserve the chain from both branches — i.e. also change line 578 to
`failure.addSuppressed(again); failure = again;` — so that when the exception
is eventually logged or thrown via `retryQueue`/`failOnError`, the full cause
history is visible.)
--
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]
