gnodet-bot commented on code in PR #328:
URL:
https://github.com/apache/maven-clean-plugin/pull/328#discussion_r4083036761
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -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.
+ *
+ * <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) {
+ try {
+ Files.deleteIfExists(path);
+ } catch (IOException e) {
+ remaining++;
+ }
+ }
Review Comment:
🐛 **Bug: batch retry ignores `force`**
The first pass calls `tryDeleteOnce(file, force)` which, when `force=true`,
calls `Cleaner.setWritable()` before retrying. But the retry loop calls
`Files.deleteIfExists(path)` raw — it never makes the file writable. So with
`force=true` and a read-only file that survived the first pass, the retry will
fail for exactly the same reason as the first pass did.
Fix: use `tryDeleteOnce` here too.
```suggestion
for (Path path : failures) {
if (!tryDeleteOnce(path, force)) {
remaining++;
}
}
```
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -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 subprojects
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-subproject instances.
+ */
+ private void scanForLeftovers() {
+ if (Files.isDirectory(fastDir)) {
+ try (DirectoryStream<Path> 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));
Review Comment:
⚠️ **`scanForLeftovers` hardcodes `force=false`**
`deleteInBackground(child, false, true)` always uses `force=false` for
leftover directories from previous builds. If a previous build had `force=true`
configured (read-only files are expected), those files will silently fail in
the background and show up as errors, deferring to the next build — where
`force=false` will fail again.
This is defensible as a conservative default (we don't know the previous
build's config), but it should at least be documented: if users configure
`force=true` and a build is killed mid-clean, the leftover cleanup will not
honour `force`. Consider documenting this limitation in the method Javadoc, or
storing the `force` setting in the leftover directory name / a sidecar file.
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -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 subprojects
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-subproject instances.
+ */
+ private void scanForLeftovers() {
+ if (Files.isDirectory(fastDir)) {
+ try (DirectoryStream<Path> 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) {
Review Comment:
**`fastDeleteError` is still package-private — @desruisseaux's comment not
yet resolved**
The latest commit made `Cleaner.fastDelete` and `Cleaner.fastDeleteError`
private (correctly), but `BackgroundCleaner.fastDeleteError` remains
package-private. That is actually correct — `Cleaner.fastDeleteError` delegates
to it:
```java
return backgroundCleaner.fastDeleteError(e);
```
Since `Cleaner` and `BackgroundCleaner` are in the same package, `private`
would break this. So it cannot be made private, and @desruisseaux's comment
hasn't been replied to.
Please add a reply to the review thread explaining why `private` is not
possible here, so the reviewer knows the comment was considered.
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -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 subprojects
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));
Review Comment:
**Silent config mismatch across subprojects: no warning**
`getOrCreate` uses `computeIfAbsent` — the first subproject to call it wins,
and subsequent subprojects' `fastDir` and `fastMode` values are silently
dropped. In a reactor where one subproject sets `fastMode=at-end` and another
sets `fastMode=background`, the second subproject silently gets the first's
mode.
Since these are session-level infrastructure settings (not per-file config),
treating them as first-wins is reasonable, but silent. At minimum, add a
debug-level log when a subsequent call would have used different values:
```java
static BackgroundCleaner getOrCreate(
@Nonnull Session session, @Nonnull Log logger,
@Nonnull Path fastDir, @Nonnull FastMode fastMode) {
BackgroundCleaner bc = session.getData().computeIfAbsent(KEY,
() -> new BackgroundCleaner(session, logger, fastDir, fastMode));
if (!bc.fastDir.equals(fastDir) || bc.fastMode != fastMode) {
logger.debug("BackgroundCleaner already initialized with fastDir=" +
bc.fastDir
+ ", fastMode=" + bc.fastMode + "; ignoring fastDir=" +
fastDir
+ ", fastMode=" + fastMode + " from this subproject.");
}
return bc;
}
```
This turns a silent surprise into a diagnosable configuration issue.
--
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]
