gnodet-bot commented on code in PR #328:
URL:
https://github.com/apache/maven-clean-plugin/pull/328#discussion_r4083309636
##########
src/main/java/org/apache/maven/plugins/clean/BackgroundCleaner.java:
##########
@@ -111,44 +161,80 @@ final class BackgroundCleaner extends Cleaner implements
Listener, Runnable {
private boolean disabled;
Review Comment:
⚠️ **`disabled` must be `volatile` — new data race introduced by
session-scoped sharing**
Before this PR, each module had its own `BackgroundCleaner` instance, so
`disabled` was only ever accessed from one thread. This PR makes
`BackgroundCleaner` session-scoped (shared across all modules in a parallel
`-T` build), introducing concurrent access:
- `fastDeleteError()` writes `disabled = true` — **not synchronized**
- `fastDelete()` reads `disabled` — **synchronized on `this`**
Under the Java Memory Model, a `synchronized` read only sees writes that
were also performed while holding the same monitor. Since `fastDeleteError`
writes `disabled` without holding the lock, the write is not guaranteed to be
visible to `fastDelete`. A module thread calling `fastDeleteError` could race
with another module thread calling `fastDelete`, which may still see `false`
and try to enqueue another background task.
```suggestion
private volatile boolean disabled;
```
##########
src/test/java/org/apache/maven/plugins/clean/BackgroundCleanerTest.java:
##########
@@ -0,0 +1,279 @@
+/*
+ * 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.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.
+ */
+@DisabledOnOs(OS.WINDOWS)
+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.
+ */
+ @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));
+
+ // Fire SESSION_ENDED to flush the executor.
+ Event event = mock(Event.class);
+ when(event.getType()).thenReturn(EventType.SESSION_ENDED);
+ captor.getValue().onEvent(event);
+
+ // Give the background thread a moment to finish (onEvent waits up to
1 h, but it
+ // returns as soon as the executor terminates — in practice this is
milliseconds).
+ assertFalse(exists(target), "target directory must have been deleted");
Review Comment:
🐛 **Test passes trivially — it does not exercise background deletion**
`fastDelete()` moves `target` to a staging directory inside `fastDir` (via
`Files.move`), then submits background deletion of that staging directory.
After `fastDelete()` returns, `target` no longer exists at its original path —
the `Move` already happened synchronously.
`assertFalse(exists(target))` therefore passes **before `onEvent` is even
called**, regardless of whether the background thread ever runs. This test does
not verify that `deleteInBackground` executes or completes successfully.
To actually test background deletion, check that the staging area inside
`fastDir` is empty after `onEvent` completes:
```suggestion
// After onEvent the executor has been shut down and awaited — the
staging
// directory inside fastDir must be gone (deleted by the background
thread).
try (var children = Files.newDirectoryStream(fastDir)) {
assertFalse(children.iterator().hasNext(),
"staging area must be empty after background deletion");
} catch (java.nio.file.NoSuchFileException ignored) {
// fastDir was also deleted — that's fine
}
```
##########
src/test/java/org/apache/maven/plugins/clean/BackgroundCleanerTest.java:
##########
@@ -0,0 +1,279 @@
+/*
+ * 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.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.
+ */
+@DisabledOnOs(OS.WINDOWS)
+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.
+ */
+ @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));
+
+ // Fire SESSION_ENDED to flush the executor.
+ Event event = mock(Event.class);
+ when(event.getType()).thenReturn(EventType.SESSION_ENDED);
+ captor.getValue().onEvent(event);
+
+ // Give the background thread a moment to finish (onEvent waits up to
1 h, but it
+ // returns as soon as the executor terminates — in practice this is
milliseconds).
+ assertFalse(exists(target), "target directory must have been deleted");
+ }
+
+ // -----------------------------------------------------------------------
+ // deleteInBackground — batch retry with force
+ // -----------------------------------------------------------------------
+
+ /**
+ * With {@code force=true}, a read-only file that would fail a plain
+ * {@link Files#deleteIfExists} must still be deleted: the batch-retry path
+ * must call {@code tryDeleteOnce(path, force)} (which makes the file
writable)
+ * rather than raw {@code Files.deleteIfExists}.
+ */
+ @Test
+ void batchRetryWithForceDeletesReadOnlyFile(@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--"));
+
+ 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);
+ // force=true, retryOnError=true — retry must make the file writable.
+ assertTrue(bc.fastDelete(target, true, true));
+
+ Event event = mock(Event.class);
+ when(event.getType()).thenReturn(EventType.SESSION_ENDED);
+ captor.getValue().onEvent(event);
+
+ assertFalse(exists(target), "target with read-only file must be
deleted when force=true");
+ assertFalse(exists(readOnly), "read-only file must be deleted when
force=true");
Review Comment:
🐛 **Same flaw: both assertions pass trivially after `fastDelete`, before
background deletion runs**
`target` and `readOnly` (which is inside `target`) both point to paths that
were atomically moved to the staging area by `fastDelete()`. After the move:
- `exists(target)` → `false` (original path gone)
- `exists(readOnly)` → `false` (parent directory moved with it)
Neither assertion actually exercises the `force=true` / `setWritable` code
path in `tryDeleteOnce`.
The correct way to test `force=true` batch retry is to place the read-only
file **inside the staging directory** after `fastDelete` moves it there, or —
simpler — to directly invoke `fastDelete` on a staging-area path that
`BackgroundCleaner` controls. Alternatively, use the same pattern as
`scanForLeftoversDeletesOrphanedDirectories`: create the read-only file in a
directory already inside `fastDir` and verify `fastDir` is empty after
`onEvent`.
Here is a test structure that actually exercises the retry path:
```java
// Create the read-only file inside fastDir directly (no fastDelete move)
Path fastDir = createDirectory(tempDir.resolve(".clean"));
Path staged = createDirectory(fastDir.resolve("staged-module"));
Path readOnly = createFile(staged.resolve("ro.txt"));
Files.setPosixFilePermissions(readOnly,
PosixFilePermissions.fromString("r--r--r--"));
// Use scanForLeftovers path: construct BackgroundCleaner with existing
fastDir
Log log = mock(Log.class);
ArgumentCaptor<Listener> captor = ArgumentCaptor.forClass(Listener.class);
Session session = mockSession(captor);
// getOrCreate triggers scanForLeftovers which queues staged for background
deletion
BackgroundCleaner.getOrCreate(session, log, fastDir, FastMode.BACKGROUND);
Event event = mock(Event.class);
when(event.getType()).thenReturn(EventType.SESSION_ENDED);
captor.getValue().onEvent(event);
assertFalse(exists(staged), "staged directory with read-only file must be
deleted when force=false");
```
(Note: `scanForLeftovers` uses `force=false`, so to test `force=true` you
would need to call `fastDelete(staged, true, true)` directly on a path inside
`fastDir`.)
--
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]
