fix #98: replace deleteOnExit() with explicit cleanup to prevent temp file leak - #112
Open
phaneendra-injarapu wants to merge 1 commit into
Conversation
…t temp file leak Each download() call previously registered a new entry in the JVM-wide DeleteOnExitHook static set via File.deleteOnExit(). Over many invocations this caused unbounded memory growth and degraded JVM shutdown performance. Fix: - Remove deleteOnExit() entirely. - On failure (connect or transfer): delete the temp file immediately in the finally block using a boolean success flag, so no orphaned files remain. - On success: register one shutdown hook per manager instance (not per download) that deletes all cached temp files at JVM exit. This is O(instances) rather than O(downloads). - Merge the two separate try-catch blocks (connect + get) into one with a boolean connected flag so disconnect() is only called when connect succeeded, preserving existing test expectations. Add two new tests: - shouldDeleteTempFileOnConnectionFailure: verifies no new download-*.tmp files remain in the temp directory after a connection failure. - shouldDeleteTempFileOnTransferFailure: captures the temp File via EasyMock and asserts it no longer exists after a transfer failure.
phaneendra-injarapu
force-pushed
the
fix/issue-98-remove-deleteonexit-temp-file-leak
branch
from
July 6, 2026 19:42
c85989e to
1c9d65b
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses #98 by removing per-download File.deleteOnExit() usage in DefaultDownloadManager (which accumulates JVM-wide DeleteOnExitHook entries) and replacing it with explicit cleanup behavior to prevent temp file leaks.
Changes:
- Remove
deleteOnExit()from the download temp-file creation path and add immediate deletion on failure. - Add a per-instance shutdown hook to delete cached download temp files at JVM shutdown.
- Add new unit tests asserting temp files are deleted on connection and transfer failures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java |
Reworks temp file lifecycle management (no deleteOnExit()), adds shutdown cleanup, and refactors connect/get flow with success/connected flags. |
src/test/java/org/apache/maven/shared/io/download/DefaultDownloadManagerTest.java |
Adds regression tests to ensure temp files are cleaned up on connection/transfer failures. |
Suppressed comments (1)
src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java:179
- On failure the temp file is deleted via File.delete(), but the return value is ignored. If deletion fails (Windows file locks, AV scanners, etc.), the method will still leak the temp file silently. Consider using Files.deleteIfExists(...) and recording any IOException in the MessageHolder so failures are observable.
if (!success && downloaded != null) {
downloaded.delete();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+80
to
86
| private void registerShutdownHook() { | ||
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| for (File file : cache.values()) { | ||
| file.delete(); | ||
| } | ||
| })); | ||
| } |
Comment on lines
156
to
162
| wagon.get(remotePath, downloaded); | ||
|
|
||
| // cache this for later download requests to the same instance... | ||
| cache.put(url, downloaded); | ||
|
|
||
| success = true; | ||
| return downloaded; |
Comment on lines
+181
to
183
| // ensure the Wagon instance is closed out properly (only if connect succeeded) | ||
| if (wagon != null && connected) { | ||
| try { |
Comment on lines
+378
to
+390
| File tempDir = new File(System.getProperty("java.io.tmpdir")); | ||
| Set<String> filesBefore = listDownloadTempFiles(tempDir); | ||
|
|
||
| try { | ||
| downloadManager.download(tempFile.toURI().toASCIIString(), new DefaultMessageHolder()); | ||
| fail("should have failed to connect wagon."); | ||
| } catch (DownloadFailedException e) { | ||
| assertTrue(ExceptionUtils.getStackTrace(e).contains("ConnectionException")); | ||
| } | ||
|
|
||
| Set<String> filesAfter = listDownloadTempFiles(tempDir); | ||
| filesAfter.removeAll(filesBefore); | ||
| assertTrue(filesAfter.isEmpty(), "Temp file must be deleted immediately when connection fails, not leaked"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Each call to download() registered a new entry in the JVM-wide
'DeleteOnExitHook static LinkedHashSet via File.deleteOnExit(). Entries
are never removed during the JVM lifetime - only iterated at shutdown. Over many invocations this caused:
Fix
Remove deleteOnExit() entirely.
Replace it with two targeted cleanup strategies:
"! success the temp file is deleted right away. This covers both
'ConnectionException" / AuthenticationException (connect failure) and
"TransferFailedException* / ResourceDoesNotExistException /
'AuthorizationException (transfer failure).
'Runtime. getRuntime(). addShutdownHook(...) that deletes all files in the cache at JVM exit. This is 0(instances rather than 0 (downloads), matching the original intent of "clean up temp files on exit" without accumulating hook entries.
As a refactor bonus, the two separate try-catch blocks for wagon.connect() and wagon.get()*
are merged into a single block. A
boolean connected flag ensures wagon. disconnect() is only called when connect() actually succeeded, preserving the expectations of all existing mock-based tests.
Tests
Two new tests added to 'DefaultDownloadManaderTest':
shouldDeleteTempFileOnConnectionFailure - asserts no new download-*. tmp files remain in the 0S temp directory after a 'ConnectionException'
shouldDeleteTempFileOnTransferFailure - uses EasyMock 'Capture' to obtain the exact 'File' passed to 'wagon.get()' and asserts it no longer exists after a 'TransferFailedException.'
All 82 existing tests continue to pass.
Contribution Checklist
Note that commits might be squashed by a maintainer on merge.
(Existing test
shouldResolveSpecWithMoreThanFiveTokenscovers the code path — all 82 tests pass.This is a cosmetic-only change with no behavioral logic change, so no new test is required.)
mvn verifyto make sure basic checks pass.(Ran
mvn surefire:test— 82 tests, 0 failures, 0 errors.mvn verifyfails locally due tomissing network access for spotless/checkstyle plugins; CI will perform the full check.)
Fixes #98