gnodet commented on code in PR #347:
URL:
https://github.com/apache/maven-clean-plugin/pull/347#discussion_r4110361767
##########
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:
Fixed in e90cf6e. The visitor now tracks depth relative to the staged root
and passes it through to `tryDeleteOnce` → `setWritable`, so it can walk up to
the parent directory when the file itself is already writable.
##########
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:
Fixed in e90cf6e. The test now makes the *directory* read-only (`r-xr-xr-x`)
instead of the file, so `Files.deleteIfExists` on the file inside throws
`AccessDeniedException` and the `force` branch is actually exercised.
##########
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:
Fixed in e90cf6e. Error reporting now includes up to 10 failing paths with
their full path. Also fixed the interrupt case: collected failures are now
reported before returning instead of being silently discarded.
--
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]
