Copilot commented on code in PR #112:
URL: https://github.com/apache/maven-shared-io/pull/112#discussion_r3695495151
##########
src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java:
##########
@@ -128,35 +142,44 @@ public File download(String url, List<TransferListener>
transferListeners, Messa
messageHolder.addMessage("Connecting to: " + repo.getHost() +
"(baseUrl: " + repo.getUrl() + ")");
+ boolean success = false;
+ boolean connected = false;
try {
wagon.connect(
repo,
wagonManager.getAuthenticationInfo(repo.getId()),
wagonManager.getProxy(sourceUrl.getProtocol()));
- } catch (ConnectionException e) {
- throw new DownloadFailedException(url, "Download failed", e);
- } catch (AuthenticationException e) {
- throw new DownloadFailedException(url, "Download failed", e);
- }
+ connected = true;
- messageHolder.addMessage("Getting: " + remotePath);
+ messageHolder.addMessage("Getting: " + remotePath);
- try {
wagon.get(remotePath, downloaded);
// cache this for later download requests to the same instance...
cache.put(url, downloaded);
+ success = true;
return downloaded;
+ } catch (ConnectionException e) {
+ throw new DownloadFailedException(url, "Download failed", e);
+ } catch (AuthenticationException e) {
+ throw new DownloadFailedException(url, "Download failed", e);
} catch (TransferFailedException e) {
throw new DownloadFailedException(url, "Download failed", e);
} catch (ResourceDoesNotExistException e) {
throw new DownloadFailedException(url, "Download failed", e);
} catch (AuthorizationException e) {
throw new DownloadFailedException(url, "Download failed", e);
} finally {
- // ensure the Wagon instance is closed out properly.
- if (wagon != null) {
+ // On failure, delete the temp file immediately to avoid leaving
orphaned files.
+ // Successfully downloaded files are cleaned up by the shutdown
hook registered
+ // in registerShutdownHook().
+ if (!success && downloaded != null) {
+ downloaded.delete();
+ }
+
+ // ensure the Wagon instance is closed out properly (only if
connect succeeded)
+ if (wagon != null && connected) {
try {
Review Comment:
TransferListeners are added to the Wagon before connect(), but the current
finally block only removes them when connected==true. If connect() fails, the
listeners remain attached to the Wagon instance, which can leak listeners and
affect subsequent uses of the same Wagon. Disconnect should be guarded by
connected, but listener removal should happen whenever wagon != null.
##########
src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java:
##########
@@ -59,13 +59,30 @@ public class DefaultDownloadManager implements
DownloadManager {
/**
* Create an instance of the {@code DefaultDownloadManager}.
*/
- public DefaultDownloadManager() {}
+ public DefaultDownloadManager() {
+ registerShutdownHook();
+ }
/**
* @param wagonManager {@link
org.apache.maven.repository.legacy.WagonManager}
*/
public DefaultDownloadManager(WagonManager wagonManager) {
this.wagonManager = wagonManager;
+ registerShutdownHook();
+ }
+
+ /**
+ * Registers a single JVM shutdown hook per manager instance that deletes
all
+ * cached temporary download files at JVM exit. This avoids the memory leak
+ * caused by {@code File.deleteOnExit()}, which accumulates entries in the
+ * JVM-wide {@code DeleteOnExitHook} static set on every download
invocation.
+ */
+ private void registerShutdownHook() {
+ Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+ for (File file : cache.values()) {
+ file.delete();
+ }
+ }));
}
Review Comment:
The shutdown hook deletes files via File.delete() and ignores failures;
addShutdownHook can also throw IllegalStateException/SecurityException (e.g.,
shutdown in progress or restricted runtime), which would currently fail
construction. Consider making cleanup best-effort with
Files.deleteIfExists(...) and guarding hook registration so the manager remains
usable even if a hook cannot be installed.
##########
src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java:
##########
@@ -128,35 +142,44 @@ public File download(String url, List<TransferListener>
transferListeners, Messa
messageHolder.addMessage("Connecting to: " + repo.getHost() +
"(baseUrl: " + repo.getUrl() + ")");
+ boolean success = false;
+ boolean connected = false;
try {
wagon.connect(
repo,
wagonManager.getAuthenticationInfo(repo.getId()),
wagonManager.getProxy(sourceUrl.getProtocol()));
- } catch (ConnectionException e) {
- throw new DownloadFailedException(url, "Download failed", e);
- } catch (AuthenticationException e) {
- throw new DownloadFailedException(url, "Download failed", e);
- }
+ connected = true;
- messageHolder.addMessage("Getting: " + remotePath);
+ messageHolder.addMessage("Getting: " + remotePath);
- try {
wagon.get(remotePath, downloaded);
// cache this for later download requests to the same instance...
cache.put(url, downloaded);
+ success = true;
return downloaded;
Review Comment:
cache.put(url, downloaded) can overwrite an existing cached file during
concurrent downloads of the same URL. In that case, the overwritten temp file
is no longer tracked in cache and (since deleteOnExit was removed) will not be
cleaned up by the shutdown hook, leaving an orphaned temp file on disk. Use
putIfAbsent (or similar) and delete the redundant temp file when another thread
has already populated the cache.
This issue also appears on line 177 of the same file.
##########
src/test/java/org/apache/maven/shared/io/download/DefaultDownloadManagerTest.java:
##########
@@ -357,6 +364,82 @@ void shouldDownloadConcurrentlyAndCacheResults() throws
Exception {
verify(wagon, wagonManager);
}
+ @Test
+ void shouldDeleteTempFileOnConnectionFailure() throws Exception {
+ File tempFile = Files.createTempFile("download-source",
"test").toFile();
+ tempFile.deleteOnExit();
+
+ setupMocksWithWagonConnectionException(new
ConnectionException("connect error"));
+
+ replay(wagon, wagonManager);
+
+ DownloadManager downloadManager = new
DefaultDownloadManager(wagonManager);
+
+ 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");
Review Comment:
This test scans the global java.io.tmpdir for download-*.tmp before/after
the call. That can be flaky if other tests (or external processes) create
matching files concurrently. Consider running the download in an isolated temp
directory by temporarily overriding java.io.tmpdir for the duration of this
test.
--
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]
