slawekjaranowski commented on code in PR #347:
URL:
https://github.com/apache/maven-clean-plugin/pull/347#discussion_r4111513996
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -255,18 +344,111 @@ boolean fastDelete(Path baseDir) throws IOException {
}
/**
- * Deletes the given directory without logging messages and without
throwing {@link IOException}.
- * The exceptions are stored for reporting after the end of the session.
+ * Deletes the given directory in a background thread using batch retry.
+ * Unlike the foreground {@link Cleaner}, this method does not call {@code
System.gc()}
+ * or sleep per file, avoiding the stop-the-world JVM pauses that caused
the performance
+ * regression described in MCLEAN-102.
+ *
+ * <p>The deletion proceeds in two passes:</p>
+ * <ol>
+ * <li><b>Walk:</b> traverse the file tree and attempt to delete each
file/directory once.
+ * Failures are silently collected without retrying.</li>
+ * <li><b>Batch retry:</b> if {@code retryOnError} is enabled and there
were failures,
+ * sleep once ({@value #BATCH_RETRY_DELAY_MS}ms) to let external
processes release
+ * file locks, then retry all failures together.</li>
+ * </ol>
+ *
+ * <p>Any files that still cannot be deleted after the batch retry will be
cleaned up
+ * by the {@linkplain #scanForLeftovers() leftover scan} on the next
build.</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.
+ * This method is designed to run in the background executor thread. It
does not share
+ * any mutable state with the main thread except through {@link
#errorOccurred(IOException)},
+ * which is synchronized.
+ *
+ * @param dir the directory to delete
+ * @param force whether to force the deletion of read-only files
+ * @param retryOnError whether to undertake a batch retry of failed
deletions
*/
- private void deleteSilently(final Path dir) {
+ private void deleteInBackground(Path dir, boolean force, boolean
retryOnError) {
+ logger.debug("Deleting " + dir + " in background.");
+ List<Path> failures = new ArrayList<>();
try {
- Files.walkFileTree(dir, Set.of(), Integer.MAX_VALUE, new
Cleaner(this));
+ Files.walkFileTree(dir, Set.of(), Integer.MAX_VALUE, new
SimpleFileVisitor<>() {
+ @Override
+ public FileVisitResult visitFile(Path file,
BasicFileAttributes attrs) {
+ if (!tryDeleteOnce(file, force)) {
+ failures.add(file);
+ }
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult postVisitDirectory(Path d, IOException
exc) {
+ if (!tryDeleteOnce(d, force)) {
+ failures.add(d);
+ }
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult visitFileFailed(Path file, IOException
exc) {
+ failures.add(file);
+ return FileVisitResult.CONTINUE;
+ }
+ });
} catch (IOException e) {
errorOccurred(e);
+ return;
+ }
+ if (!failures.isEmpty() && retryOnError) {
+ try {
+ Thread.sleep(BATCH_RETRY_DELAY_MS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ return;
+ }
+ int remaining = 0;
+ for (Path path : failures) {
+ if (!tryDeleteOnce(path, force)) {
+ remaining++;
+ }
+ }
+ if (remaining > 0) {
+ errorOccurred(new IOException("Failed to delete " + remaining
+ " path(s) during background clean;"
Review Comment:
Recording the current state so this thread is not misleading.
The path list landed and works well — from a real run against `fca73f3`:
```
[WARNING] Errors during background file deletion.
java.io.IOException: Failed to delete 3 path(s) during background clean
…/.fastdir/probe-…/read-only-dir/read-only.properties
…/.fastdir/probe-…/read-only-dir
…/.fastdir/probe-…
```
That is a real improvement over the bare count.
What is still missing is the *reason*. `failures` is a `List<Path>` and
`tryDeleteOnce` returns a `boolean`, so every exception is dropped on the
floor. The user cannot tell an `AccessDeniedException` from a
`DirectoryNotEmptyException` from a Windows file lock — and that is the part
that decides what to do about it. Worth noting that `visitFileFailed(Path file,
IOException exc)` is already handed the exception and discards it, so some of
this is available for free.
Carrying a small `record Failure(Path path, IOException cause)` instead of a
bare `Path` would cover it, and `buildFailureException` could attach the first
N causes as suppressed exceptions.
Whether that is worth doing in this PR is entirely your call — the reporting
is in a much better place than where it started, so I have no objection either
way. Leaving the thread open for you to close if you would rather not change it.
--
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]
