slawekjaranowski commented on code in PR #347:
URL:
https://github.com/apache/maven-clean-plugin/pull/347#discussion_r4111579853
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -255,18 +381,156 @@ 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<>() {
+ /**
+ * Current depth relative to the root directory, used by
{@link Cleaner#setWritable}
+ * to walk up to the parent when the file itself is already
writable.
+ */
+ int depth;
+
+ @Override
+ public FileVisitResult preVisitDirectory(Path d,
BasicFileAttributes attrs) {
+ if (ON_WINDOWS && attrs.isOther()) {
+ // MCLEAN-93: NTFS junctions have isDirectory() and
isOther() attributes set.
+ // Delete the junction itself and skip its contents to
avoid deleting the
+ // contents of the junction target, which may be
outside the project.
+ if (!tryDeleteOnce(d, force, depth)) {
+ failures.add(d);
+ }
+ return FileVisitResult.SKIP_SUBTREE;
+ }
+ depth++;
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult visitFile(Path file,
BasicFileAttributes attrs) {
+ if (!tryDeleteOnce(file, force, depth)) {
+ failures.add(file);
+ }
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult postVisitDirectory(Path d, IOException
exc) {
+ depth--;
+ if (!tryDeleteOnce(d, force, depth)) {
+ 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();
+ // Report collected failures before returning.
+ errorOccurred(buildFailureException(failures));
+ return;
+ }
+ List<Path> remaining = new ArrayList<>();
+ for (Path path : failures) {
+ if (!tryDeleteOnce(path, force, 0)) {
+ remaining.add(path);
+ }
+ }
+ if (!remaining.isEmpty()) {
+ errorOccurred(buildFailureException(remaining));
+ }
+ } else if (!failures.isEmpty()) {
+ errorOccurred(buildFailureException(failures));
+ }
+ }
+
+ /**
+ * Builds an {@link IOException} that reports the failing paths (capped at
10) and total count.
+ *
+ * @param failures the list of paths that could not be deleted
+ * @return an exception describing the failures
+ */
+ private static IOException buildFailureException(List<Path> failures) {
+ StringBuilder sb = new StringBuilder("Failed to delete ")
+ .append(failures.size())
+ .append(" path(s) during background clean");
+ int limit = Math.min(failures.size(), 10);
+ for (int i = 0; i < limit; i++) {
+ sb.append("\n ").append(failures.get(i));
+ }
+ if (failures.size() > limit) {
+ sb.append("\n ... and ").append(failures.size() - limit).append("
more");
+ }
+ return new IOException(sb.toString());
+ }
+
+ /**
+ * Tries to delete a single file or directory once, without retry delays
or {@code System.gc()}.
+ * If {@code force} is enabled and deletion fails with {@link
AccessDeniedException},
+ * the file (or its parent directory) is made writable and deletion is
retried immediately (once).
+ *
+ * @param file the file or directory to delete
+ * @param force whether to make read-only files writable before retrying
+ * @param currentDepth the depth of the file relative to the staged root,
used by
+ * {@link Cleaner#setWritable} to walk up to the
parent directory
+ * @return {@code true} if the file was deleted or did not exist
+ */
+ private static boolean tryDeleteOnce(Path file, boolean force, int
currentDepth) {
+ try {
+ Files.deleteIfExists(file);
+ return true;
+ } catch (AccessDeniedException e) {
+ if (force) {
+ try {
+ Cleaner.setWritable(file, currentDepth);
Review Comment:
Thanks — the other two points from this round are solid. The `Failure`
record carries the causes through nicely (`AccessDeniedException` vs
`DirectoryNotEmptyException` now distinguishable in the output, causes attached
as suppressed), and the batch retry passes `failure.depth()`.
This one, though, does not work in a real build. I tested all three fast
modes against `783d5e2`, same project, `force=false`, `retryOnError=false`,
`failOnError=true`, `target/read-only-dir` at `dr-xr-xr-x` holding a file at
`r--r--r--`:
| configuration | exit code | outcome |
|---|---|---|
| `fast=false` | **1** | BUILD FAILURE |
| `fast=true`, `fastMode=background` | **0** | BUILD SUCCESS |
| `fast=true`, `fastMode=at-end` | **0** | BUILD SUCCESS |
| `fast=true`, `fastMode=defer` | **0** | BUILD SUCCESS |
`run()` is invoked from `onEvent`, i.e. from a session `Listener`, and Maven
catches whatever a listener throws:
```
[WARNING] Failed to notify spy org.apache.maven.internal.impl.EventSpyImpl:
Failed to clean project: Failed to delete 3 path(s) during background clean
…/read-only-dir/read-only.properties: java.nio.file.AccessDeniedException:
…
…
[INFO] BUILD SUCCESS
```
So a listener structurally cannot fail the build — the
`UncheckedIOException` is downgraded to a warning about an internal spy
notification, which reads as a Maven problem rather than a failed clean.
The new tests do not catch this because they call the listener directly:
```java
captor.getValue().onEvent(event);
assertThrows(UncheckedIOException.class, ...)
```
which bypasses exactly the dispatch that swallows the exception.
`BackgroundCleanerTest` is 10/10 green and CI is 8/8 while the feature does not
work in any mode.
Two side effects worth noting either way:
- **The error is now printed twice.** `errorOccurred` stores into `errors`
unconditionally *and* into `fatalErrors` when `failOnError`, and it is the same
exception instance, so `run()` logs it as a warning and then throws it. With
more than one failing directory the duplication gets worse:
`fatalErrors.addSuppressed(e2)` and `errors.addSuppressed(e2)` hit the same
object, so each subsequent failure is added to the suppressed list twice.
- **In `defer` mode `BUILD SUCCESS` is printed before the deletion even
runs**, and the exception goes into the `executor.submit(this)` `Future` where
nothing observes it — no spy warning at all. `failOnError` cannot work there by
construction.
My suggestion is to go with option 1 from my earlier comment and treat this
as out of scope:
1. Document that `failOnError` has no effect when `fast=true` — in the
`fast` and `failOnError` parameter Javadoc and on the plugin site.
2. **Revert the `throw` from `run()`**, since as it stands it only produces
the misleading spy warning and the duplicate output, without failing anything.
3. Open a follow-up issue for honouring `failOnError` in fast mode properly.
It needs a mechanism that can actually fail a build, which a session-end
listener is not — for example having `CleanMojo` check the accumulated fatal
errors at the start of the next module's execution and throw
`MojoExecutionException`, or, when `failOnError=true`, waiting for that
module's deletion to complete inside `CleanMojo.execute()` and giving up the
asynchrony for that configuration. Both have real trade-offs and neither
belongs in this PR.
Happy to be argued out of this if you see a way to fail the build from
session end that I have missed — but I would rather not merge a `throw` that
only ever surfaces as `Failed to notify spy`.
--
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]
