gnodet commented on code in PR #345:
URL:
https://github.com/apache/maven-clean-plugin/pull/345#discussion_r4110368984
##########
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:
Fixed in 6722499. Batch retry now builds a map of still-failing paths and
suppresses directory warnings when a child path also failed — only the leaf
cause is reported. Test updated to assert exactly 1 warning (the file), not the
directory cascade.
--
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]
