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.
c85989e to
1c9d65b
Compare
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