gnodet-bot commented on code in PR #345:
URL:
https://github.com/apache/maven-clean-plugin/pull/345#discussion_r4107302942
##########
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:
✅ **gnodet-bot finding #1 (interrupt contract): addressed.** The `retryQueue
= null; return;` correctly stops the retry loop on interrupt.
However, two issues remain from @slawekjaranowski's review:
1. The `BlockingQueue.take()` reference in this comment is wrong —
`BackgroundCleaner` uses `ExecutorService`, not `BlockingQueue`.
2. The method returns normally after interrupt with no warning logged, so
`CleanMojo` treats the clean as successful even though queued paths were
abandoned.
##########
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:
✅ **gnodet-bot finding #2 (exception chaining): partially addressed.**
`addSuppressed` is now called, which is an improvement. However,
@slawekjaranowski's deeper concern remains: `failure.addSuppressed(again);
failure = again;` attaches `again` to the *old* `failure` object, then replaces
`failure` with `again`. If the loop iterates a second time, the original
exception (with its suppressed `again`) becomes unreachable from the current
`failure`.
@slawekjaranowski suggested `again.addSuppressed(failure); failure = again;`
which keeps the full chain reachable. In practice this rarely iterates more
than once on POSIX (as @slawekjaranowski noted), but the logic is still
inverted.
--
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]
