gnodet commented on code in PR #347:
URL:
https://github.com/apache/maven-clean-plugin/pull/347#discussion_r4110362708
##########
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:
Fixed in e90cf6e. Changed from `debug` to `warn`. Also added a Javadoc
section to the class doc that clearly documents what is session-scoped
first-wins (`fastDir`, `fastMode`), what is per-subproject (`force`,
`retryOnError`), and what is session-wide side effect (`disabled`). The PR
description table has been inaccurate — corrected the Javadoc rather than the
PR description, since the code is the source of truth.
##########
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:
Fixed in e90cf6e. The constructor now only initializes fields.
`registerListener(this)` and `scanForLeftovers()` are moved into a separate
`init()` method, called by `getOrCreate()` immediately after
`computeIfAbsent()` returns (using a `boolean[] created` flag to ensure it runs
exactly once). This avoids both the ConcurrentHashMap re-entrant deadlock
hazard and the `this`-escape from the constructor.
--
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]
