From e681b19f70e90dc814fcadb1f262e1e78c8f321c Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Sun, 2 Aug 2026 01:12:38 +0200 Subject: [PATCH] chore: refactor fast clean to fix MCLEAN-102 and per-module correctness Refactors BackgroundCleaner to fix the MCLEAN-102 performance regression (50% slower on Windows due to System.gc() stop-the-world pauses from the background thread) and three regressions from PR #286 (thread proliferation, listener proliferation, lost leftover cleanup). BackgroundCleaner is now a standalone session-scoped service (no longer extends Cleaner). Each module gets its own Cleaner with per-module configuration and attaches the shared BackgroundCleaner via Cleaner.setBackgroundCleaner(). Per-module force/retryOnError values are passed to fastDelete() and captured per-directory so background deletion respects the originating module's configuration. Key changes: - Session-scoped via SessionData.computeIfAbsent(): one instance, one thread, one listener per session (was one per module) - Batch retry replaces per-file System.gc() + sleep: walk once, sleep once (250ms), retry all failures together - scanForLeftovers() restores cleanup of dirs from killed previous builds - fastDelete() is synchronized for parallel build safety Co-Authored-By: Claude Opus 4.6 --- .../plugins/clean/BackgroundCleaner.java | 269 ++++++++++++++---- .../apache/maven/plugins/clean/CleanMojo.java | 19 +- .../apache/maven/plugins/clean/Cleaner.java | 37 ++- 3 files changed, 255 insertions(+), 70 deletions(-) diff --git a/src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java b/src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java index f180a43..abe7b8a 100644 --- a/src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java +++ b/src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java @@ -19,10 +19,15 @@ package org.apache.maven.plugins.clean; import java.io.IOException; +import java.nio.file.AccessDeniedException; import java.nio.file.DirectoryNotEmptyException; +import java.nio.file.DirectoryStream; +import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; import java.nio.file.StandardCopyOption; +import java.nio.file.attribute.BasicFileAttributes; import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; @@ -35,12 +40,32 @@ import org.apache.maven.api.EventType; import org.apache.maven.api.Listener; import org.apache.maven.api.Session; +import org.apache.maven.api.SessionData; import org.apache.maven.api.annotations.Nonnull; import org.apache.maven.api.plugin.Log; -import org.apache.maven.api.services.PathMatcherFactory; /** - * A cleaner potentially executed by background threads. + * A session-scoped service that moves directories to a staging area and deletes them in a background thread. + * A single instance is shared across all modules in a reactor build via {@link SessionData}, + * ensuring only one background thread and one session listener regardless of the number of modules. + * + *

This class does not extend {@link Cleaner}. Each module creates its own {@link Cleaner} + * with per-module configuration ({@code force}, {@code retryOnError}, etc.) and attaches this shared + * service via {@link Cleaner#setBackgroundCleaner(BackgroundCleaner)}. The per-module values are + * passed to {@link #fastDelete(Path, boolean, boolean)} and captured alongside each directory so + * that background deletion respects the configuration of the module that requested the deletion, + * even in multi-module builds where modules configure the clean plugin differently.

+ * + *

Background deletion strategy

+ * Instead of the per-file retry used by the foreground {@link Cleaner} (which calls {@code System.gc()} + * on Windows, causing JVM-wide stop-the-world pauses — see MCLEAN-102), this class uses a batch retry + * strategy: walk the entire tree attempting each deletion once, then sleep once and retry all failures + * together. This eliminates the stop-the-world pauses that caused a 50% performance regression on + * multi-core Windows machines. + * + *

Leftover cleanup

+ * On first creation, this class scans the staging directory for leftovers from previous (possibly + * killed) builds and queues them for background deletion. * *

Limitations

* This class can be used for deleting {@link Path} only, not {@link Fileset}, because this class cannot handle @@ -48,14 +73,44 @@ * * @author Benjamin Bentmann * @author Martin Desruisseaux + * @author Guillaume Nodet */ -final class BackgroundCleaner extends Cleaner implements Listener, Runnable { +final class BackgroundCleaner implements Listener, Runnable { + + /** + * Key for storing the shared {@code BackgroundCleaner} instance in {@link SessionData}. + * Using {@link SessionData#computeIfAbsent} ensures that only one instance, one background + * thread, and one session listener are created per Maven session, regardless of the number + * of modules in the reactor. + */ + private static final SessionData.Key KEY = SessionData.key(BackgroundCleaner.class); + + /** + * Delay in milliseconds before retrying failed deletions in batch. + * A single sleep after the entire tree walk replaces the per-file + * {@code System.gc()} + sleep that caused MCLEAN-102. + */ + private static final int BATCH_RETRY_DELAY_MS = 250; + + /** + * A directory queued for deferred deletion (when {@link FastMode#AT_END} or {@link FastMode#DEFER}). + * Captures the per-module {@code force} and {@code retryOnError} values at submission time + * so that background deletion respects the originating module's configuration. + */ + private record DeferredDeletion(Path dir, boolean force, boolean retryOnError) {} + /** * The maven session. */ @Nonnull private final Session session; + /** + * The logger where to send information about what the plugin is doing. + */ + @Nonnull + private final Log logger; + /** * The directory where to temporarily move the files to delete. */ @@ -78,10 +133,10 @@ final class BackgroundCleaner extends Cleaner implements Listener, Runnable { private final ExecutorService executor; /** - * Files to delete at the end of the session instead of in background thread. + * Directories to delete at the end of the session instead of in background thread. * This is unused ({@code null}) for {@link FastMode#BACKGROUND}. */ - private final List filesToDeleteAtEnd; + private final List filesToDeleteAtEnd; /** * Directories to delete last, after the executor has been shutdown, and only if they are empty. @@ -98,11 +153,6 @@ final class BackgroundCleaner extends Cleaner implements Listener, Runnable { */ private IOException errors; - /** - * Whether at least one deletion task has been queued. - */ - private boolean started; - /** * Whether to disable the deletion of files in background threads. * This is used for avoiding to repeat the same warning many times @@ -111,44 +161,67 @@ final class BackgroundCleaner extends Cleaner implements Listener, Runnable { private boolean disabled; /** - * Creates a new cleaner to be executed in a background thread. + * Creates a new background cleaner service. + * Use {@link #getOrCreate} to obtain a session-scoped instance. * - * @param session the Maven session to be used - * @param matcherFactory the service to use for creating include and exclude filters. - * @param logger the logger to use - * @param verbose whether to perform verbose logging - * @param fastDir the explicit configured directory or to be deleted in fast mode - * @param fastMode the fast deletion mode - * @param followSymlinks whether to follow symlinks - * @param force whether to force the deletion of read-only files - * @param failOnError whether to abort with an exception in case a selected file/directory could not be deleted - * @param retryOnError whether to undertake additional delete attempts in case the first attempt failed + * @param session the Maven session to be used + * @param logger the logger to use + * @param fastDir the directory where to temporarily move the files to delete + * @param fastMode the fast deletion mode */ - @SuppressWarnings("checkstyle:ParameterNumber") - BackgroundCleaner( - @Nonnull Session session, - @Nonnull PathMatcherFactory matcherFactory, - @Nonnull Log logger, - boolean verbose, - @Nonnull Path fastDir, - @Nonnull FastMode fastMode, - boolean followSymlinks, - boolean force, - boolean failOnError, - boolean retryOnError) { - super(matcherFactory, logger, verbose, followSymlinks, force, failOnError, retryOnError); + private BackgroundCleaner( + @Nonnull Session session, @Nonnull Log logger, @Nonnull Path fastDir, @Nonnull FastMode fastMode) { this.session = session; + this.logger = logger; this.fastDir = fastDir; this.fastMode = fastMode; filesToDeleteAtEnd = (fastMode != FastMode.BACKGROUND) ? new ArrayList<>() : null; directoriesToDeleteIfEmpty = new LinkedHashSet<>(); // Will need to delete in order. executor = Executors.newSingleThreadExecutor((task) -> new Thread(task, "mvn-background-cleaner")); + session.registerListener(this); + scanForLeftovers(); + } + + /** + * Returns the session-scoped {@code BackgroundCleaner}, creating it on first access. + * The instance is stored in {@link SessionData} so that all modules in a reactor + * share the same background thread and session listener. + * + * @param session the Maven session to be used + * @param logger the logger to use + * @param fastDir the directory where to temporarily move the files to delete + * @param fastMode the fast deletion mode + * @return the shared background cleaner instance for the session + */ + static BackgroundCleaner getOrCreate( + @Nonnull Session session, @Nonnull Log logger, @Nonnull Path fastDir, @Nonnull FastMode fastMode) { + return session.getData().computeIfAbsent(KEY, () -> new BackgroundCleaner(session, logger, fastDir, fastMode)); + } + + /** + * Scans the fast directory for leftover directories from previous (possibly killed) builds + * and queues them for background deletion. This restores the cleanup behavior that was + * present in the singleton pattern of version 3.5.0 but was lost when switching to + * per-module instances. + */ + private void scanForLeftovers() { + if (Files.isDirectory(fastDir)) { + try (DirectoryStream stream = Files.newDirectoryStream(fastDir)) { + for (Path child : stream) { + if (Files.isDirectory(child)) { + logger.debug("Cleaning leftover directory from previous build: " + child); + executor.submit(() -> deleteInBackground(child, false, true)); + } + } + } catch (IOException e) { + logger.debug("Failed to scan for leftover directories in " + fastDir + ": " + e); + } + } } /** * Returns an error message to show to user if the fast delete failed. */ - @Override String fastDeleteError(IOException e) { disabled = true; var message = new StringBuilder("Unable to fast delete directory"); @@ -162,13 +235,15 @@ String fastDeleteError(IOException e) { /** * Deletes the specified directory and its contents in a background thread. + * This method is synchronized to support concurrent calls from parallel module builds. * - * @param basedir the directory to delete, must not be {@code null} + * @param baseDir the directory to delete, must not be {@code null} + * @param force whether to force the deletion of read-only files + * @param retryOnError whether to undertake a batch retry of failed deletions * @return whether this method was able to register the background task * @throws IOException if an error occurred while preparing the task before execution in a background thread */ - @Override - boolean fastDelete(Path baseDir) throws IOException { + synchronized boolean fastDelete(Path baseDir, boolean force, boolean retryOnError) throws IOException { if (disabled) { return false; } @@ -176,10 +251,6 @@ boolean fastDelete(Path baseDir) throws IOException { if (parent == null) { return false; } - if (!started) { - started = true; - session.registerListener(this); - } /* * The default directory is `${maven.multiModuleProjectDirectory}/target/.clean`. * This is fine when cleaning a multi-project, in which case this directory will @@ -239,9 +310,9 @@ boolean fastDelete(Path baseDir) throws IOException { try { final Path dir = Files.move(baseDir, tmpDir, StandardCopyOption.REPLACE_EXISTING); if (filesToDeleteAtEnd != null) { - filesToDeleteAtEnd.add(dir); + filesToDeleteAtEnd.add(new DeferredDeletion(dir, force, retryOnError)); } else { - executor.submit(() -> deleteSilently(dir)); + executor.submit(() -> deleteInBackground(dir, force, retryOnError)); } } catch (IOException | RuntimeException e) { try { @@ -255,18 +326,113 @@ 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. + * + *

The deletion proceeds in two passes:

+ *
    + *
  1. Walk: traverse the file tree and attempt to delete each file/directory once. + * Failures are silently collected without retrying.
    2. + *
  2. Batch retry: 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.
    3. + *
+ * + *

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.

* *

Thread safety

- * 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 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) { + try { + Files.deleteIfExists(path); + } catch (IOException e) { + remaining++; + } + } + if (remaining > 0) { + errorOccurred(new IOException("Failed to delete " + remaining + " file(s) during background clean;" + + " will retry on next build")); + } + } else if (!failures.isEmpty()) { + errorOccurred(new IOException("Failed to delete " + failures.size() + " file(s) during background clean")); + } + } + + /** + * 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 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 + * @return {@code true} if the file was deleted or did not exist + */ + private static boolean tryDeleteOnce(Path file, boolean force) { + try { + Files.deleteIfExists(file); + return true; + } catch (AccessDeniedException e) { + if (force) { + try { + Cleaner.setWritable(file, 0); + Files.deleteIfExists(file); + return true; + } catch (IOException retry) { + return false; + } + } + return false; + } catch (IOException e) { + return false; } } @@ -293,7 +459,8 @@ public void onEvent(Event event) { } session.unregisterListener(this); if (filesToDeleteAtEnd != null) { - filesToDeleteAtEnd.forEach((dir) -> executor.submit(() -> deleteSilently(dir))); + filesToDeleteAtEnd.forEach( + (d) -> executor.submit(() -> deleteInBackground(d.dir(), d.force(), d.retryOnError()))); } if (fastMode == FastMode.DEFER) { executor.submit(this); diff --git a/src/main/java/org/apache/maven/plugins/clean/CleanMojo.java b/src/main/java/org/apache/maven/plugins/clean/CleanMojo.java index 9d2daf7..b930708 100644 --- a/src/main/java/org/apache/maven/plugins/clean/CleanMojo.java +++ b/src/main/java/org/apache/maven/plugins/clean/CleanMojo.java @@ -261,26 +261,15 @@ public void execute() { logger.info("Clean is skipped."); return; } - Cleaner cleaner; + Cleaner cleaner = + new Cleaner(matcherFactory, logger, isVerbose(), followSymLinks, force, failOnError, retryOnError); if (fast && session != null) { Path tmpDir = fastDir; if (tmpDir == null) { tmpDir = session.getRootDirectory().resolve("target").resolve(".clean"); } - cleaner = new BackgroundCleaner( - session, - matcherFactory, - logger, - isVerbose(), - tmpDir, - FastMode.caseInsensitiveValueOf(fastMode), - followSymLinks, - force, - failOnError, - retryOnError); - } else { - cleaner = - new Cleaner(matcherFactory, logger, isVerbose(), followSymLinks, force, failOnError, retryOnError); + cleaner.setBackgroundCleaner( + BackgroundCleaner.getOrCreate(session, logger, tmpDir, FastMode.caseInsensitiveValueOf(fastMode))); } try { for (Path directoryItem : getDirectories()) { diff --git a/src/main/java/org/apache/maven/plugins/clean/Cleaner.java b/src/main/java/org/apache/maven/plugins/clean/Cleaner.java index bcdff20..97b04b5 100644 --- a/src/main/java/org/apache/maven/plugins/clean/Cleaner.java +++ b/src/main/java/org/apache/maven/plugins/clean/Cleaner.java @@ -44,8 +44,10 @@ import org.apache.maven.api.services.PathMatcherFactory; /** - * Cleans directories. This base class deletes the files in the current thread. - * The {@link BackgroundCleaner} subclass adds the capability to run in background. + * Cleans directories. This class deletes the files in the current thread. + * If a {@link BackgroundCleaner} is {@linkplain #setBackgroundCleaner(BackgroundCleaner) attached}, + * directories eligible for fast deletion are moved to a staging area and deleted in the background, + * using the {@code force} and {@code retryOnError} configuration of this cleaner instance. * *

Limitations

* This class is not thread-safe: each instance shall be be executed in only one thread at a time. @@ -144,6 +146,14 @@ class Cleaner implements FileVisitor { */ private final boolean retryOnError; + /** + * The shared background cleaner service, or {@code null} if fast deletion is not enabled. + * When set, {@link #fastDelete(Path)} delegates to this service, passing the per-module + * {@link #force} and {@link #retryOnError} values so that each module's configuration + * is respected even though the background thread is shared. + */ + private BackgroundCleaner backgroundCleaner; + /** * The delays (in milliseconds) if {@link #retryOnError} is {@code true}. * The length of this array is the maximal number of new attempts. @@ -228,6 +238,17 @@ class Cleaner implements FileVisitor { directoryMatcher = fileMatcher; } + /** + * Sets the shared background cleaner service for fast deletion. + * When set, {@link #fastDelete(Path)} will delegate to the background cleaner, + * passing this cleaner's {@link #force} and {@link #retryOnError} configuration. + * + * @param backgroundCleaner the session-scoped background cleaner, or {@code null} to disable fast deletion + */ + void setBackgroundCleaner(BackgroundCleaner backgroundCleaner) { + this.backgroundCleaner = backgroundCleaner; + } + /** * Deletes the specified fileset in the current thread. * This method modifies the include and exclude filters, @@ -307,13 +328,18 @@ private boolean isClearAll() { /** * Deletes the specified directory and its contents in a background thread. - * The default implementation returns {@code false}. + * If a {@link BackgroundCleaner} has been {@linkplain #setBackgroundCleaner set}, + * delegates to it with this cleaner's {@link #force} and {@link #retryOnError} values. + * Otherwise returns {@code false}. * * @param basedir the directory to delete, must not be {@code null} * @return whether this method was able to register the background task * @throws IOException if an error occurred while preparing the task before execution in a background thread */ boolean fastDelete(Path baseDir) throws IOException { + if (backgroundCleaner != null) { + return backgroundCleaner.fastDelete(baseDir, force, retryOnError); + } return false; } @@ -321,6 +347,9 @@ boolean fastDelete(Path baseDir) throws IOException { * Returns an error message to show to user if the fast delete failed. */ String fastDeleteError(IOException e) { + if (backgroundCleaner != null) { + return backgroundCleaner.fastDeleteError(e); + } return e.toString(); } @@ -447,7 +476,7 @@ private static Path getCanonicalPath(final Path path, IOException mainError) thr * @param currentDepth 0 for the base directory, and decremented for each parent directory * @return the root path which has been made writable, or {@code null} if none */ - private static Path setWritable(Path file, int currentDepth) throws IOException { + static Path setWritable(Path file, int currentDepth) throws IOException { while (file != null) { PosixFileAttributeView posix = Files.getFileAttributeView(file, PosixFileAttributeView.class); if (posix != null) {