gnodet opened a new pull request, #347: URL: https://github.com/apache/maven-clean-plugin/pull/347
## Summary Refactors the fast clean (`-Dmaven.clean.fast=true`) `BackgroundCleaner` to fix the MCLEAN-102 performance regression and three regressions introduced by the per-module refactoring in PR #286. Fixes https://github.com/apache/maven-clean-plugin/issues/120 > ℹ️ This PR replaces #328 (same content, branch renamed to fix Jenkins CI) ### What this fixes | # | Bug | Root Cause | Fix | |---|------|------------|-----| | 1 | **MCLEAN-102** — fast mode 50% slower on 12-core Windows | `System.gc()` stop-the-world per file + per-file retry sleeps from background thread | Batch retry: walk once without retry, sleep once (250ms), retry all failures | | 2 | **Thread proliferation** — 500 threads in 500-module reactor | Per-module `BackgroundCleaner`, each with its own `ExecutorService` | Session-scoped instance via `SessionData.computeIfAbsent()` | | 3 | **500 session listeners** | Per-module `session.registerListener(this)` | Single listener registered in constructor | | 4 | **Leftover cleanup lost** | `init()` scan removed in PR #286 when singleton was dropped | Restored: `scanForLeftovers()` queues leftover dirs for background deletion | ### Design `BackgroundCleaner` no longer extends `Cleaner`. It is a standalone session-scoped service that owns only the shared infrastructure (executor, session listener, staging directory, fast mode). Each module creates its own `Cleaner` with per-module configuration (`force`, `retryOnError`, `followSymlinks`, etc.) and attaches the shared `BackgroundCleaner` via `Cleaner.setBackgroundCleaner()`. The per-module `force`/`retryOnError` values are passed to `fastDelete()` and captured alongside each directory (via a `DeferredDeletion` record for at-end mode) so that background deletion respects the originating module's configuration — even in multi-module builds where modules configure the clean plugin differently. ### Changes - **`BackgroundCleaner.java`** (major rewrite): - No longer extends `Cleaner` — standalone session-scoped service - `SessionData.Key<BackgroundCleaner>` + `getOrCreate()` factory — one instance, one thread, one listener per session - `fastDelete(Path, boolean force, boolean retryOnError)` — per-module config captured per-call - `deleteInBackground()` — uses `SimpleFileVisitor` with batch retry instead of per-file `System.gc()` + sleep - `DeferredDeletion` record — captures per-module config for at-end/defer modes - `scanForLeftovers()` — scans fast directory for dirs left by killed previous builds - `fastDelete()` is `synchronized` for parallel build safety (`-T`) - **`Cleaner.java`**: - Added `backgroundCleaner` field + `setBackgroundCleaner()` setter - `fastDelete()` and `fastDeleteError()` delegate to `BackgroundCleaner`, passing per-instance `force`/`retryOnError` - `setWritable()`: `private static` → `static` (package-private) so `BackgroundCleaner` can reuse it - **`CleanMojo.java`**: - Always creates a per-module `Cleaner` with that module's configuration - If `fast=true`, gets/creates the shared `BackgroundCleaner` and attaches it ### Before vs After (500-module reactor) | Metric | Before | After | |--------|--------|-------| | Background threads | **500** | **1** | | Session listeners | **500** | **1** | | `System.gc()` calls (Windows) | thousands | **0** | | Per-file sleeps (background) | 50–1050ms × N | **0** | | Batch sleeps | 0 | **1 × 250ms per directory** | | Leftover cleanup | ❌ | ✅ | | Per-module config respected | ✅ (separate instances) | ✅ (per-call params) | ### Design notes - **Foreground path unchanged**: the traditional `Cleaner.tryDelete()` keeps `System.gc()` + per-file retry — it runs in the main thread where it can't cause stop-the-world on other threads, and it may genuinely need to release JVM-held file handles. - **Batch retry respects `retryOnError`**: if user sets `-Dmaven.clean.retryOnError=false`, no retry happens even in background. - **Remaining failures deferred to next build**: any files that survive the batch retry will be cleaned up by the leftover scan on the next `fast=true` build. ## Test plan - [x] `mvn test` passes (15 tests, 0 failures) - [x] CI integration tests pass (fast-delete, fast-delete-default) - [ ] Manual test on Windows with large reactor to verify MCLEAN-102 fix - [ ] Manual test: kill build mid-clean, verify leftovers cleaned on next run - [ ] Manual test with `-T 4C` parallel build to verify session-scoped sharing -- 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]
