slawekjaranowski commented on code in PR #347:
URL:
https://github.com/apache/maven-clean-plugin/pull/347#discussion_r4107110416
##########
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<>() {
Review Comment:
MCLEAN-93 regression: this visitor has no `preVisitDirectory`, so the NTFS
junction guard is gone.
Before this PR the background walk used `new Cleaner(this)` as the visitor,
so `Cleaner.preVisitDirectory` applied:
```java
if (ON_WINDOWS && !followSymlinks && attrs.isOther()) {
// MCLEAN-93: NTFS junctions have isDirectory() and isOther() attributes
set.
visitFile(dir, attrs);
return FileVisitResult.SKIP_SUBTREE;
}
```
A junction inside `target` survives the `Files.move` into the staging area,
and Java does not report it as a symbolic link, so without that guard
`walkFileTree` recurses into it and deletes the contents of the junction
*target* — outside `target`, and outside the project.
Could you add a `preVisitDirectory` override mirroring that logic (delete
the junction itself, `SKIP_SUBTREE`), and an IT creating one with `mklink /J`?
##########
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:
This loses the information the user needs. The old path logged
`logger.warn("Failed to delete " + file, failure)` for each path; now all that
reaches the user is a count, via the single `logger.warn("Errors during
background file deletion.", errors)` in `run()` — no paths, no causes. When
fast clean leaves files behind there is nothing to act on. Could the failing
paths (capped, say at 10) and their exceptions be kept?
Related, just above: an interrupt during the batch sleep returns without
reporting the collected failures at all.
##########
src/test/java/org/apache/maven/plugins/clean/BackgroundCleanerTest.java:
##########
@@ -0,0 +1,306 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.plugins.clean;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Supplier;
+
+import org.apache.maven.api.Event;
+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.plugin.Log;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.DisabledOnOs;
+import org.junit.jupiter.api.condition.OS;
+import org.junit.jupiter.api.io.TempDir;
+import org.mockito.ArgumentCaptor;
+
+import static java.nio.file.Files.createDirectory;
+import static java.nio.file.Files.createFile;
+import static java.nio.file.Files.exists;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Unit tests for {@link BackgroundCleaner}'s new batch-retry, session-scoping,
+ * and leftover-scan logic introduced in the fast-clean refactor.
+ */
+class BackgroundCleanerTest {
+
+ /**
+ * Minimal in-memory {@link SessionData} that supports {@link
#computeIfAbsent}.
+ */
+ private static class FakeSessionData implements SessionData {
+ private final Map<Key<?>, Object> store = new HashMap<>();
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public <T> T get(Key<T> key) {
+ return (T) store.get(key);
+ }
+
+ @Override
+ public <T> void set(Key<T> key, T value) {
+ store.put(key, value);
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public <T> T computeIfAbsent(Key<T> key, Supplier<T> supplier) {
+ return (T) store.computeIfAbsent(key, k -> supplier.get());
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public <T> boolean replace(Key<T> key, T expected, T value) {
+ T current = (T) store.get(key);
+ if (current == expected) {
+ store.put(key, value);
+ return true;
+ }
+ return false;
+ }
+ }
+
+ /**
+ * Creates a mock {@link Session} backed by a real {@link FakeSessionData}
so that
+ * {@code computeIfAbsent} works correctly across multiple calls to {@code
getOrCreate}.
+ *
+ * <p>The returned captor can be used to retrieve the registered {@link
Listener} and
+ * fire synthetic events at it.</p>
+ */
+ private Session mockSession(ArgumentCaptor<Listener> listenerCaptor) {
+ Session session = mock(Session.class);
+ FakeSessionData data = new FakeSessionData();
+ when(session.getData()).thenReturn(data);
+ if (listenerCaptor != null) {
+ // Capture any listener registered during BackgroundCleaner
construction.
+
org.mockito.Mockito.doNothing().when(session).registerListener(listenerCaptor.capture());
+ }
+ return session;
+ }
+
+ // -----------------------------------------------------------------------
+ // getOrCreate — session-scoping
+ // -----------------------------------------------------------------------
+
+ /**
+ * Two calls to {@link BackgroundCleaner#getOrCreate} with the same
parameters must
+ * return the same instance (session-scoped singleton).
+ */
+ @Test
+ void getOrCreateReturnsSameInstance(@TempDir Path tempDir) throws
IOException {
+ Path fastDir = tempDir.resolve(".clean");
+ Log log = mock(Log.class);
+ ArgumentCaptor<Listener> captor =
ArgumentCaptor.forClass(Listener.class);
+ Session session = mockSession(captor);
+
+ BackgroundCleaner bc1 = BackgroundCleaner.getOrCreate(session, log,
fastDir, FastMode.BACKGROUND);
+ BackgroundCleaner bc2 = BackgroundCleaner.getOrCreate(session, log,
fastDir, FastMode.BACKGROUND);
+
+ assertSame(bc1, bc2, "getOrCreate must return the same instance for
the same session");
+ // Constructor registers exactly one listener
+ verify(session, atLeastOnce()).registerListener(any(Listener.class));
+ }
+
+ /**
+ * When a second subproject calls {@link BackgroundCleaner#getOrCreate}
with a different
+ * {@code fastMode}, a debug-level warning must be emitted (first-wins
semantics).
+ */
+ @Test
+ void getOrCreateLogsDebugOnConfigMismatch(@TempDir Path tempDir) throws
IOException {
+ Path fastDir = tempDir.resolve(".clean");
+ Log log = mock(Log.class);
+ Session session = mockSession(null);
+
+ BackgroundCleaner.getOrCreate(session, log, fastDir,
FastMode.BACKGROUND);
+ // Second call with different fastMode — should log a debug message.
+ BackgroundCleaner.getOrCreate(session, log, fastDir, FastMode.AT_END);
+
+ verify(log, atLeastOnce()).debug(any(CharSequence.class));
+ }
+
+ /**
+ * When a second subproject calls {@link BackgroundCleaner#getOrCreate}
with the same
+ * parameters, no debug warning about a mismatch must be emitted.
+ */
+ @Test
+ void getOrCreateNoDebugWhenSameConfig(@TempDir Path tempDir) throws
IOException {
+ Path fastDir = tempDir.resolve(".clean");
+ Log log = mock(Log.class);
+ Session session = mockSession(null);
+
+ BackgroundCleaner.getOrCreate(session, log, fastDir,
FastMode.BACKGROUND);
+ BackgroundCleaner.getOrCreate(session, log, fastDir,
FastMode.BACKGROUND);
+
+ verify(log, never()).debug(any(CharSequence.class));
+ }
+
+ // -----------------------------------------------------------------------
+ // deleteInBackground — basic deletion
+ // -----------------------------------------------------------------------
+
+ /**
+ * Files placed in the staging area via {@link
BackgroundCleaner#fastDelete} must be
+ * deleted in the background before the session ends.
+ *
+ * <p><b>Note on assertion strategy:</b> {@code fastDelete} moves {@code
target} to a staging
+ * directory under {@code fastDir} <em>synchronously</em>, so {@code
exists(target)} becomes
+ * {@code false} immediately — before any background deletion runs. The
meaningful assertion is
+ * that the staging area inside {@code fastDir} is empty after {@code
onEvent} has drained the
+ * executor (i.e. background deletion actually completed).</p>
+ */
+ @Test
+ void fastDeleteRemovesDirectoryInBackground(@TempDir Path tempDir) throws
Exception {
+ Path fastDir = tempDir.resolve(".clean");
+ Path target = createDirectory(tempDir.resolve("target"));
+ createFile(target.resolve("file.txt"));
+
+ Log log = mock(Log.class);
+ ArgumentCaptor<Listener> captor =
ArgumentCaptor.forClass(Listener.class);
+ Session session = mockSession(captor);
+
+ BackgroundCleaner bc = BackgroundCleaner.getOrCreate(session, log,
fastDir, FastMode.BACKGROUND);
+ assertTrue(bc.fastDelete(target, false, true));
+
+ // After fastDelete(), target has been moved to fastDir — the staging
area exists.
+ assertTrue(exists(fastDir), "staging directory must exist after
fastDelete");
+
+ // Fire SESSION_ENDED: onEvent shuts down the executor and waits for
completion.
+ Event event = mock(Event.class);
+ when(event.getType()).thenReturn(EventType.SESSION_ENDED);
+ captor.getValue().onEvent(event);
+
+ // After onEvent returns, the background thread has finished and run()
has cleaned up.
+ // fastDir itself is deleted by run() when it is empty
(directoriesToDeleteIfEmpty).
+ assertFalse(exists(fastDir), "staging directory must be deleted after
background clean completes");
+ }
+
+ // -----------------------------------------------------------------------
+ // deleteInBackground — batch retry with force
+ // -----------------------------------------------------------------------
+
+ /**
+ * With {@code force=true}, {@code tryDeleteOnce} must handle a read-only
file by calling
+ * {@link Cleaner#setWritable} and retrying immediately (first-pass force
logic), so
+ * that the file is successfully deleted in the background.
+ *
+ * <p><b>Note on what this test covers:</b> This exercises the first-pass
+ * {@code force=true} handling in {@code tryDeleteOnce} (i.e., {@code
setWritable} +
+ * immediate retry on {@code AccessDeniedException}). The second-pass
batch-retry
+ * sleep-and-loop is only reachable when a file fails even after {@code
setWritable}
+ * (e.g. a file held open by another process on Windows) and is not
exercised here.</p>
+ *
+ * <p><b>Note on assertion strategy:</b> {@code fastDelete} moves the
entire {@code target}
+ * tree (including the read-only file) to a staging directory under {@code
fastDir}
+ * <em>synchronously</em>. After that move, neither {@code target} nor
{@code readOnly}
+ * exist at their original paths — the assertions would pass trivially.
The meaningful
+ * check is that the staging area itself is empty after {@code onEvent}
completes, which
+ * proves that {@code tryDeleteOnce(path, true)} successfully handled the
read-only file
+ * inside the staging tree.</p>
+ */
+ @Test
+ @DisabledOnOs(OS.WINDOWS)
+ void forceDeleteHandlesReadOnlyFile(@TempDir Path tempDir) throws
Exception {
+ Path fastDir = tempDir.resolve(".clean");
+ Path target = createDirectory(tempDir.resolve("target"));
+ Path readOnly = createFile(target.resolve("ro.txt"));
+ // Make the file read-only so the first pass fails.
+ Files.setPosixFilePermissions(readOnly,
PosixFilePermissions.fromString("r--r--r--"));
Review Comment:
This test does not exercise `force`. The file is `r--r--r--` but its parent
stays writable, and on POSIX `Files.deleteIfExists` succeeds in that situation
— no `AccessDeniedException` is thrown, so `tryDeleteOnce` returns `true` on
the first attempt and the `force` branch is never entered. The test passes
identically with `force=false`.
Making the *directory* read-only instead would both exercise the `force`
branch and cover the `setWritable(file, 0)` issue above.
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -98,57 +153,93 @@ 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
* when the {@link #fastDir} directory does not exist.
+ *
+ * <p>This field is written by {@link #fastDeleteError(IOException)}
without holding any lock
+ * (to avoid a lock-ordering risk), and read by the synchronized {@link
#fastDelete} method.
+ * Declaring it {@code volatile} ensures that the write is immediately
visible to all threads
+ * without requiring the reader to hold the same monitor as the writer.</p>
*/
- private boolean disabled;
+ private volatile 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) {
+ BackgroundCleaner bc =
+ session.getData().computeIfAbsent(KEY, () -> new
BackgroundCleaner(session, logger, fastDir, fastMode));
+ if (!bc.fastDir.equals(fastDir) || bc.fastMode != fastMode) {
Review Comment:
`fastDir` and `fastMode` are now first-module-wins, and that is reported
only at `debug`. Before this PR each module had its own `BackgroundCleaner`
honouring its own values, so this is a silent behaviour change for a reactor
where a module overrides either — `warn` would be more appropriate.
While here: the description's "Per-module config respected ✅" holds for
`force`/`retryOnError` only. `fastDir`/`fastMode` are first-wins as above;
`verbose` is dropped entirely (the old visitor logged every deleted file via
`listDeletedFiles`/`logDelete`, now there is one `debug` line per staged
directory); and `disabled` is now session-wide, so one module's
`fastDeleteError` disables fast clean for every remaining module. Worth
correcting the table.
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -98,57 +153,93 @@ 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
* when the {@link #fastDir} directory does not exist.
+ *
+ * <p>This field is written by {@link #fastDeleteError(IOException)}
without holding any lock
+ * (to avoid a lock-ordering risk), and read by the synchronized {@link
#fastDelete} method.
+ * Declaring it {@code volatile} ensures that the write is immediately
visible to all threads
+ * without requiring the reader to hold the same monitor as the writer.</p>
*/
- private boolean disabled;
+ private volatile 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) {
+ 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;
+ }
+
+ /**
+ * 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.
+ *
+ * <p><b>Limitation:</b> leftovers are always deleted with {@code
force=false}.
+ * Because the previous build's configuration is not persisted, we cannot
know
+ * whether it used {@code force=true}. As a consequence, read-only files
that
+ * survived a killed build will not be force-deleted here; they will
remain until
+ * the user runs a new clean with {@code force=true}.</p>
+ */
+ 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:
Good to have this back. One case worth considering: two builds sharing
`${maven.multiModuleProjectDirectory}/target/.clean` — an IDE build and a CLI
build, or two `mvn` invocations — will have the second one delete the first
one's in-flight staging directories, producing spurious errors in both. This
matches 3.5.0 behaviour so it is not new, but an age filter (only directories
older than a few minutes) or a marker file would make it safe.
Also: a leftover that genuinely cannot be deleted will now produce "Errors
during background file deletion." on every subsequent build.
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -98,57 +153,93 @@ 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
* when the {@link #fastDir} directory does not exist.
+ *
+ * <p>This field is written by {@link #fastDeleteError(IOException)}
without holding any lock
+ * (to avoid a lock-ordering risk), and read by the synchronized {@link
#fastDelete} method.
+ * Declaring it {@code volatile} ensures that the write is immediately
visible to all threads
+ * without requiring the reader to hold the same monitor as the writer.</p>
*/
- private boolean disabled;
+ private volatile 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);
Review Comment:
These two run from the constructor, which executes inside the
`SessionData.computeIfAbsent` mapping function. `DefaultSessionData` delegates
to `ConcurrentHashMap.computeIfAbsent`, where any re-entrant access to the same
map from the mapping function is a documented deadlock /
`IllegalStateException` hazard — and `this` escapes to the session before
construction completes.
Keeping the mapping function to just `new BackgroundCleaner(...)` and doing
the listener registration plus the leftover scan after `computeIfAbsent`
returns (idempotently) would avoid both.
##########
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;"
+ + " will retry on next build"));
+ }
+ } else if (!failures.isEmpty()) {
+ errorOccurred(new IOException("Failed to delete " +
failures.size() + " path(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);
Review Comment:
`force=true` loses the read-only-parent case here.
With `currentDepth = 0`, `setWritable` only ever tries the file itself: if
the file is already writable, `permissions.add(OWNER_WRITE)` returns `false`,
then `--currentDepth < 0` breaks the loop and the method returns `null`. The
parent directory is never made writable.
That is the case `force` exists for, per the Javadoc of `Cleaner.force`:
> Note that on Linux, `Files.delete(Path)` and `Files.deleteIfExists(Path)`
delete read-only files but throw `AccessDeniedException` if the directory
containing the file is read-only.
The foreground `tryDelete` passes the real `currentDepth` so it can walk up;
here it cannot. Since the visitor already knows the depth relative to the
staged root, could that be threaded through to `tryDeleteOnce`?
--
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]
