diff --git a/src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java b/src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java index b9efbc1..94d914c 100644 --- a/src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java +++ b/src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java @@ -29,6 +29,7 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import org.apache.commons.io.FileUtils; import org.apache.maven.artifact.manager.WagonManager; import org.apache.maven.shared.io.logging.MessageHolder; import org.apache.maven.wagon.ConnectionException; @@ -56,6 +57,33 @@ public class DefaultDownloadManager implements DownloadManager { private Map cache = new ConcurrentHashMap<>(); + /** + * Shared parent of all download directories. One JVM shutdown hook deletes it. + */ + private static File downloadRoot; + + /** + * Whether the shutdown hook registration was already attempted. Keeps it to one hook. + */ + private static boolean shutdownHookAttempted; + + /** + * Number of shutdown hooks registered. + */ + private static int registeredShutdownHooks; + + /** + * @return how many JVM shutdown hooks this class registered. + */ + static synchronized int registeredShutdownHooks() { + return registeredShutdownHooks; + } + + /** + * This manager's own download directory, so {@link #cleanup()} only deletes its own files. + */ + private File downloadDirectory; + /** * Create an instance of the {@code DefaultDownloadManager}. */ @@ -68,6 +96,121 @@ public DefaultDownloadManager(WagonManager wagonManager) { this.wagonManager = wagonManager; } + /** + * Deletes the temporary files downloaded through this manager and empties its cache, so that + * subsequent requests download again. Calling this is optional: the files are removed when the + * JVM exits anyway. It is worth calling in a long-lived JVM, such as a Maven daemon or an + * embedded build, once the downloaded files are no longer needed. Do not call it while a + * download is in progress on another thread, as that download writes into the directory being + * removed. + */ + public void cleanup() { + cache.clear(); + + File directory; + synchronized (this) { + directory = downloadDirectory; + downloadDirectory = null; + } + + if (directory != null) { + FileUtils.deleteQuietly(directory); + } + } + + /** + * @return the directory of this manager, creating it, the shared root and the shutdown hook that + * removes the root on first use. + * @throws IOException if the directory cannot be created. + */ + private synchronized File downloadDirectory() throws IOException { + if (downloadDirectory == null || !downloadDirectory.isDirectory()) { + downloadDirectory = Files.createTempDirectory(downloadRoot().toPath(), "manager-") + .toFile(); + } + + return downloadDirectory; + } + + private static synchronized File downloadRoot() throws IOException { + // Recreate the root if something else deleted it, such as a temp dir sweeper. + if (downloadRoot == null || !downloadRoot.isDirectory()) { + downloadRoot = + Files.createTempDirectory("maven-shared-io-downloads-").toFile(); + registerShutdownHook(); + } + + return downloadRoot; + } + + /** + * Registers, at most once, the hook that removes {@link #downloadRoot} at JVM exit. Registering + * one hook for the lifetime of the class, instead of one per root, is what keeps the JVM's hook + * set from growing: a root that a temp dir sweeper removes is replaced without a second hook. + */ + private static void registerShutdownHook() { + if (shutdownHookAttempted) { + return; + } + + // Set before the attempt, so a failure is not retried on every recreation of the root. + shutdownHookAttempted = true; + + preloadDeleteClasses(); + + Thread hook = new Thread(DefaultDownloadManager::deleteDownloadRoot, "maven-shared-io-download-cleanup"); + + // The hook lives until JVM exit, so give it as few references as possible. The inherited + // context class loader is a plugin class realm in Maven and would be kept alive for the + // whole run of a long-lived JVM. + hook.setContextClassLoader(null); + + try { + Runtime.getRuntime().addShutdownHook(hook); + registeredShutdownHooks++; + } catch (IllegalStateException e) { + // Already shutting down, so no hook can be added. Leave the files to the OS temp cleanup. + } catch (SecurityException e) { + // Not allowed to register a hook. Downloading must still work, so fall back to the + // operating system's temp directory cleanup, as above. + } + } + + /** + * Deletes a throwaway directory tree so the classes {@link #deleteDownloadRoot()} needs are + * loaded up front. At JVM exit the class loader may be closed, {@link FileUtils} would fail to + * load and the hook would delete nothing. + * Called while holding the class lock, so {@link #downloadRoot} is the root just created. + */ + private static void preloadDeleteClasses() { + File warmUp = new File(downloadRoot, ".warm-up"); + + try { + Files.createDirectories(warmUp.toPath().resolve("nested")); + Files.createFile(warmUp.toPath().resolve("nested/file")); + } catch (IOException e) { + // Nothing to walk, so fewer classes load. The hook is no worse off. + } + + FileUtils.deleteQuietly(warmUp); + } + + /** + * Deletes the current download root, ignoring failures. Called only by the shutdown hook. + * The lock only reads {@link #downloadRoot}; the deletion itself need not be exclusive, because + * a concurrent {@link #cleanup()} also uses {@link FileUtils#deleteQuietly(File)}. + */ + private static void deleteDownloadRoot() { + File root; + synchronized (DefaultDownloadManager.class) { + root = downloadRoot; + } + + if (root != null) { + FileUtils.deleteQuietly(root); + } + } + /** {@inheritDoc} */ public File download(String url, MessageHolder messageHolder) throws DownloadFailedException { return download(url, Collections.emptyList(), messageHolder); @@ -76,7 +219,7 @@ public File download(String url, MessageHolder messageHolder) throws DownloadFai /** {@inheritDoc} */ public File download(String url, List transferListeners, MessageHolder messageHolder) throws DownloadFailedException { - File downloaded = (File) cache.get(url); + File downloaded = cache.get(url); if (downloaded != null && downloaded.exists()) { messageHolder.addMessage("Using cached download: " + downloaded.getAbsolutePath()); @@ -103,11 +246,10 @@ public File download(String url, List transferListeners, Messa messageHolder.addMessage("Using wagon: " + wagon + " to download: " + url); try { - // create the landing file in /tmp for the downloaded source archive - downloaded = Files.createTempFile("download-", null).toFile(); - - // delete when the JVM exits, to avoid polluting the temp dir... - downloaded.deleteOnExit(); + // create the landing file for the downloaded source archive, in the temp directory that + // is removed as a whole at JVM exit, so no per-file exit hook is needed. + downloaded = Files.createTempFile(downloadDirectory().toPath(), "download-", null) + .toFile(); } catch (IOException e) { throw new DownloadFailedException(url, "Failed to create temporary file target for download.", e); } @@ -139,26 +281,40 @@ public File download(String url, List transferListeners, Messa messageHolder.addMessage("Connecting to: " + repo.getHost() + "(baseUrl: " + repo.getUrl() + ")"); + boolean retainTempFile = 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); + File cached = cache.putIfAbsent(url, downloaded); + + if (cached != null && cached.exists()) { + // Another thread cached this URL first. Return its file, which callers may already + // be using, and let the finally block delete this copy. + return cached; + } + + if (cached != null) { + // The cached file is gone, so replace the entry with this one. Losing this race is + // harmless: either file is valid and both are deleted with the temp directory. + cache.replace(url, cached, downloaded); + } + retainTempFile = 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) { @@ -166,16 +322,26 @@ public File download(String url, List transferListeners, Messa } catch (AuthorizationException e) { throw new DownloadFailedException(url, "Download failed", e); } finally { - // ensure the Wagon instance is closed out properly. - if (wagon != null) { - try { - messageHolder.addMessage("Disconnecting."); + // Delete the temp file unless the cache now holds it. Covers a failed download and a + // lost race to cache the same URL. + if (!retainTempFile) { + downloaded.delete(); + } - wagon.disconnect(); - } catch (ConnectionException e) { - messageHolder.addMessage("Failed to disconnect wagon for: " + url, e); + if (wagon != null) { + // Only disconnect if the connection was actually established. + if (connected) { + try { + messageHolder.addMessage("Disconnecting."); + + wagon.disconnect(); + } catch (ConnectionException e) { + messageHolder.addMessage("Failed to disconnect wagon for: " + url, e); + } } + // Listeners are added before connecting, so remove them even if connecting failed. + // Otherwise they stay attached to a Wagon that may be reused. for (Iterator it = transferListeners.iterator(); it.hasNext(); ) { wagon.removeTransferListener(it.next()); } diff --git a/src/test/java/org/apache/maven/shared/io/download/DefaultDownloadManagerTest.java b/src/test/java/org/apache/maven/shared/io/download/DefaultDownloadManagerTest.java index b0fb7af..992be52 100644 --- a/src/test/java/org/apache/maven/shared/io/download/DefaultDownloadManagerTest.java +++ b/src/test/java/org/apache/maven/shared/io/download/DefaultDownloadManagerTest.java @@ -19,13 +19,25 @@ package org.apache.maven.shared.io.download; import java.io.File; +import java.io.IOException; +import java.lang.reflect.Method; +import java.net.URL; +import java.net.URLClassLoader; +import java.nio.file.DirectoryStream; import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.maven.artifact.manager.WagonManager; @@ -57,7 +69,9 @@ import static org.easymock.EasyMock.verify; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -511,6 +525,491 @@ 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); + + Set filesBefore = listDownloadTempFiles(); + + 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 filesAfter = listDownloadTempFiles(); + filesAfter.removeAll(filesBefore); + assertTrue(filesAfter.isEmpty(), "Temp file must be deleted immediately when connection fails, not leaked"); + + verify(wagon, wagonManager); + } + + @Test + void shouldDeleteTempFileOnTransferFailure() throws Exception { + File tempFile = Files.createTempFile("download-source", "test").toFile(); + tempFile.deleteOnExit(); + + expect(wagonManager.getWagon("file")).andReturn(wagon); + expect(wagonManager.getAuthenticationInfo(anyString())).andReturn(null); + expect(wagonManager.getProxy(anyString())).andReturn(null); + try { + wagon.connect(anyObject(Repository.class), anyObject(AuthenticationInfo.class), anyObject(ProxyInfo.class)); + } catch (ConnectionException | AuthenticationException e) { + fail("This shouldn't happen!!"); + } + + Capture capturedTempFile = newCapture(); + try { + wagon.get(anyString(), capture(capturedTempFile)); + expectLastCall().andThrow(new TransferFailedException("bad transfer")); + } catch (TransferFailedException | AuthorizationException | ResourceDoesNotExistException e) { + fail("This shouldn't happen!!"); + } + + assertDoesNotThrow(() -> wagon.disconnect(), "This shouldn't happen!!"); + + replay(wagon, wagonManager); + + DownloadManager downloadManager = new DefaultDownloadManager(wagonManager); + + try { + downloadManager.download(tempFile.toURI().toASCIIString(), new DefaultMessageHolder()); + fail("should have thrown DownloadFailedException"); + } catch (DownloadFailedException e) { + assertTrue(ExceptionUtils.getStackTrace(e).contains("TransferFailedException")); + } + + assertTrue(capturedTempFile.hasCaptured(), "wagon.get() should have been called"); + assertFalse( + capturedTempFile.getValue().exists(), + "Temp file must be deleted immediately when transfer fails, not leaked"); + + verify(wagon, wagonManager); + } + + @Test + void shouldDownloadIntoTheSharedTempDirectoryInsteadOfRegisteringDeleteOnExit() throws Exception { + File tempFile = Files.createTempFile("download-source", "test").toFile(); + tempFile.deleteOnExit(); + + expect(wagonManager.getWagon("file")).andReturn(wagon); + expect(wagonManager.getAuthenticationInfo(anyString())).andReturn(null); + expect(wagonManager.getProxy(anyString())).andReturn(null); + wagon.connect(anyObject(Repository.class), anyObject(AuthenticationInfo.class), anyObject(ProxyInfo.class)); + + Capture capturedTempFile = newCapture(); + wagon.get(anyString(), capture(capturedTempFile)); + wagon.disconnect(); + + replay(wagon, wagonManager); + + DownloadManager downloadManager = new DefaultDownloadManager(wagonManager); + + downloadManager.download(tempFile.toURI().toASCIIString(), new DefaultMessageHolder()); + + // Downloads live under one directory that a single shutdown hook removes, so the amount of + // JVM shutdown bookkeeping stays constant instead of growing with every download. + Path downloadPath = capturedTempFile.getValue().toPath().toAbsolutePath(); + Path tempRoot = Paths.get(System.getProperty("java.io.tmpdir")).toAbsolutePath(); + + assertTrue(downloadPath.startsWith(tempRoot), "Download must stay in the temp directory: " + downloadPath); + assertTrue( + tempRoot.relativize(downloadPath).getName(0).toString().startsWith("maven-shared-io-downloads-"), + "Download must land in the shared download directory: " + downloadPath); + + verify(wagon, wagonManager); + } + + @Test + void shouldDownloadAgainWhenTheCachedFileWasDeleted() throws Exception { + File tempFile = Files.createTempFile("download-source", "test").toFile(); + tempFile.deleteOnExit(); + + expect(wagonManager.getWagon("file")).andReturn(wagon).anyTimes(); + expect(wagonManager.getAuthenticationInfo(anyString())).andReturn(null).anyTimes(); + expect(wagonManager.getProxy(anyString())).andReturn(null).anyTimes(); + wagon.connect(anyObject(Repository.class), anyObject(AuthenticationInfo.class), anyObject(ProxyInfo.class)); + expectLastCall().anyTimes(); + wagon.get(anyString(), anyObject(File.class)); + expectLastCall().anyTimes(); + wagon.disconnect(); + expectLastCall().anyTimes(); + + replay(wagon, wagonManager); + + DownloadManager downloadManager = new DefaultDownloadManager(wagonManager); + + String url = tempFile.toURI().toASCIIString(); + + File first = downloadManager.download(url, new DefaultMessageHolder()); + assertTrue(first.delete(), "should have deleted the downloaded file"); + + File second = downloadManager.download(url, new DefaultMessageHolder()); + + assertTrue(second.exists(), "must not hand back the stale cache entry of a deleted file"); + assertFalse(first.equals(second), "must download to a fresh file"); + + // The stale entry has been replaced, so the next request is served from the cache again. + assertSame(second, downloadManager.download(url, new DefaultMessageHolder())); + + verify(wagon, wagonManager); + } + + @Test + void shouldDeleteDownloadedFilesOnCleanup() throws Exception { + File tempFile = Files.createTempFile("download-source", "test").toFile(); + tempFile.deleteOnExit(); + + setupDefaultMockConfiguration(); + + replay(wagon, wagonManager); + + DefaultDownloadManager downloadManager = new DefaultDownloadManager(wagonManager); + + File downloaded = downloadManager.download(tempFile.toURI().toASCIIString(), new DefaultMessageHolder()); + assertTrue(downloaded.exists()); + + downloadManager.cleanup(); + + assertFalse(downloaded.exists(), "cleanup() must delete the downloaded file"); + assertFalse(downloaded.getParentFile().exists(), "cleanup() must delete the manager's directory"); + + verify(wagon, wagonManager); + } + + @Test + void shouldStillBeUsableAfterCleanup() throws Exception { + File tempFile = Files.createTempFile("download-source", "test").toFile(); + tempFile.deleteOnExit(); + + expect(wagonManager.getWagon("file")).andReturn(wagon).anyTimes(); + expect(wagonManager.getAuthenticationInfo(anyString())).andReturn(null).anyTimes(); + expect(wagonManager.getProxy(anyString())).andReturn(null).anyTimes(); + wagon.connect(anyObject(Repository.class), anyObject(AuthenticationInfo.class), anyObject(ProxyInfo.class)); + expectLastCall().anyTimes(); + wagon.get(anyString(), anyObject(File.class)); + expectLastCall().anyTimes(); + wagon.disconnect(); + expectLastCall().anyTimes(); + + replay(wagon, wagonManager); + + DefaultDownloadManager downloadManager = new DefaultDownloadManager(wagonManager); + + String url = tempFile.toURI().toASCIIString(); + + downloadManager.download(url, new DefaultMessageHolder()); + downloadManager.cleanup(); + + File afterCleanup = downloadManager.download(url, new DefaultMessageHolder()); + + assertTrue(afterCleanup.exists(), "must download into a freshly created directory after cleanup()"); + + verify(wagon, wagonManager); + } + + @Test + void shouldNotDeleteTheFilesOfAnotherManagerOnCleanup() throws Exception { + File tempFile = Files.createTempFile("download-source", "test").toFile(); + tempFile.deleteOnExit(); + + expect(wagonManager.getWagon("file")).andReturn(wagon).anyTimes(); + expect(wagonManager.getAuthenticationInfo(anyString())).andReturn(null).anyTimes(); + expect(wagonManager.getProxy(anyString())).andReturn(null).anyTimes(); + wagon.connect(anyObject(Repository.class), anyObject(AuthenticationInfo.class), anyObject(ProxyInfo.class)); + expectLastCall().anyTimes(); + wagon.get(anyString(), anyObject(File.class)); + expectLastCall().anyTimes(); + wagon.disconnect(); + expectLastCall().anyTimes(); + + replay(wagon, wagonManager); + + String url = tempFile.toURI().toASCIIString(); + + DefaultDownloadManager first = new DefaultDownloadManager(wagonManager); + DefaultDownloadManager second = new DefaultDownloadManager(wagonManager); + + File keptFile = first.download(url, new DefaultMessageHolder()); + File droppedFile = second.download(url, new DefaultMessageHolder()); + + second.cleanup(); + + assertFalse(droppedFile.exists(), "cleanup() must delete the files of its own manager"); + assertTrue(keptFile.exists(), "cleanup() must not delete the files of another manager"); + + first.cleanup(); + + verify(wagon, wagonManager); + } + + @Test + void shouldRegisterAtMostOneShutdownHookHoweverManyDownloadsAndManagers() throws Exception { + File tempFile = Files.createTempFile("download-source", "test").toFile(); + tempFile.deleteOnExit(); + + expectAnyNumberOfDownloads(); + + String url = tempFile.toURI().toASCIIString(); + + // Trigger the first download so that the root, and its hook, exist before counting. + DefaultDownloadManager warmUp = new DefaultDownloadManager(wagonManager); + warmUp.download(url, new DefaultMessageHolder()); + + int hooksAfterFirstDownload = DefaultDownloadManager.registeredShutdownHooks(); + + assertEquals(1, hooksAfterFirstDownload, "the root must be removed by a single shutdown hook"); + + List managers = new ArrayList<>(); + + for (int i = 0; i < 25; i++) { + DefaultDownloadManager manager = new DefaultDownloadManager(wagonManager); + managers.add(manager); + + manager.download(url, new DefaultMessageHolder()); + manager.download(url + "?run=" + i, new DefaultMessageHolder()); + manager.cleanup(); + manager.download(url, new DefaultMessageHolder()); + } + + assertEquals( + hooksAfterFirstDownload, + DefaultDownloadManager.registeredShutdownHooks(), + "downloads, managers and cleanup() must not add shutdown hooks"); + + for (DefaultDownloadManager manager : managers) { + manager.cleanup(); + } + + warmUp.cleanup(); + + verify(wagon, wagonManager); + } + + @Test + void shouldNotRegisterAnotherShutdownHookWhenTheRootIsRemovedBehindOurBack() throws Exception { + File tempFile = Files.createTempFile("download-source", "test").toFile(); + tempFile.deleteOnExit(); + + expectAnyNumberOfDownloads(); + + String url = tempFile.toURI().toASCIIString(); + + DefaultDownloadManager downloadManager = new DefaultDownloadManager(wagonManager); + downloadManager.download(url, new DefaultMessageHolder()); + + int hooksBefore = DefaultDownloadManager.registeredShutdownHooks(); + + // A temp dir sweeper removes the whole root between downloads, repeatedly. + for (int i = 0; i < 25; i++) { + for (Path root : listDownloadRoots()) { + deleteRecursively(root); + } + + File downloaded = downloadManager.download(url + "?sweep=" + i, new DefaultMessageHolder()); + + assertTrue(downloaded.exists(), "must recreate the root and keep downloading after a sweep"); + } + + assertEquals( + hooksBefore, + DefaultDownloadManager.registeredShutdownHooks(), + "recreating the root must reuse the existing shutdown hook, not add one per root"); + + downloadManager.cleanup(); + + verify(wagon, wagonManager); + } + + @Test + void shouldDeleteNestedDirectoriesOnCleanupWithoutFollowingSymbolicLinks() throws Exception { + File tempFile = Files.createTempFile("download-source", "test").toFile(); + tempFile.deleteOnExit(); + + expectAnyNumberOfDownloads(); + + DefaultDownloadManager downloadManager = new DefaultDownloadManager(wagonManager); + + File downloaded = downloadManager.download(tempFile.toURI().toASCIIString(), new DefaultMessageHolder()); + + // Whatever a wagon leaves in the download directory has to go too, including a subdirectory + // and a link pointing outside the tree, whose target must survive. + Path directory = downloaded.toPath().getParent(); + Path nested = Files.createDirectories(directory.resolve("nested/deeper")); + Path nestedFile = Files.createFile(nested.resolve("leftover.tmp")); + + Path outsideTarget = Files.createTempFile("outside-target", ".tmp"); + outsideTarget.toFile().deleteOnExit(); + + Path link = directory.resolve("link-to-outside"); + boolean linkCreated; + try { + Files.createSymbolicLink(link, outsideTarget); + linkCreated = true; + } catch (IOException | UnsupportedOperationException e) { + // Some platforms need a privilege for this; the rest of the assertions still apply. + linkCreated = false; + } + + downloadManager.cleanup(); + + assertFalse(Files.exists(nestedFile), "cleanup() must delete files in nested directories"); + assertFalse(Files.exists(directory), "cleanup() must delete the download directory itself"); + + if (linkCreated) { + assertTrue(Files.exists(outsideTarget), "cleanup() must not follow a symbolic link out of the tree"); + } + + verify(wagon, wagonManager); + } + + @Test + void shouldDeleteTheRootWhenTheShutdownHookCanNoLongerLoadClasses() throws Exception { + // The hook runs at JVM exit, when the class loader that defined DefaultDownloadManager may + // already be closed, as a Maven plugin realm is at the end of a build. A class the hook only + // needs then, commons-io for one, could no longer be resolved, so the hook would fail and + // delete nothing. Load the manager in isolation, hide commons-io once the root exists, and + // check the hook still deletes: it must load what it needs before it is registered. + URL classes = DefaultDownloadManager.class + .getProtectionDomain() + .getCodeSource() + .getLocation(); + + HidingClassLoader hiding = new HidingClassLoader(DefaultDownloadManagerTest.class.getClassLoader()); + + try (URLClassLoader isolated = new URLClassLoader(new URL[] {classes}, hiding)) { + Class isolatedManager = isolated.loadClass(DefaultDownloadManager.class.getName()); + + assertSame(isolated, isolatedManager.getClassLoader(), "the manager must come from the isolated loader"); + + Method downloadDirectory = isolatedManager.getDeclaredMethod("downloadDirectory"); + downloadDirectory.setAccessible(true); + Method deleteDownloadRoot = isolatedManager.getDeclaredMethod("deleteDownloadRoot"); + deleteDownloadRoot.setAccessible(true); + + File directory = (File) + downloadDirectory.invoke(isolatedManager.getConstructor().newInstance()); + Path downloaded = Files.createFile(directory.toPath().resolve("download-0")); + Path nested = Files.createDirectories(directory.toPath().resolve("nested")); + Path nestedFile = Files.createFile(nested.resolve("leftover.tmp")); + + // Stand in for the closed plugin realm: from here on no commons-io class can be loaded. + // Classes the manager already resolved stay usable, exactly as they do in a closed realm, + // which is why the manager has to resolve them before the hook is registered. + hiding.hideCommonsIo(); + + assertThrows( + ClassNotFoundException.class, + () -> isolated.loadClass("org.apache.commons.io.monitor.FileAlterationMonitor"), + "no commons-io class may still be loaded through the isolated loader by now"); + + deleteDownloadRoot.invoke(null); + + assertFalse(Files.exists(downloaded), "the shutdown hook must delete the downloaded files"); + assertFalse(Files.exists(nestedFile), "the shutdown hook must delete nested files"); + assertFalse(Files.exists(directory.toPath()), "the shutdown hook must delete the download directory"); + assertFalse(Files.exists(directory.toPath().getParent()), "the shutdown hook must delete the root itself"); + } + } + + /** + * Hides the download package, so that the child loader has to define it itself, and hides + * commons-io from {@link #hideCommonsIo()} on, standing in for a plugin realm that is closed + * while the shutdown hook runs. Everything else comes from the test's own loader. + */ + private static final class HidingClassLoader extends ClassLoader { + + private volatile boolean commonsIoHidden; + + HidingClassLoader(ClassLoader parent) { + super(parent); + } + + void hideCommonsIo() { + commonsIoHidden = true; + } + + @Override + protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { + if (name.startsWith("org.apache.maven.shared.io.download.") + || (commonsIoHidden && name.startsWith("org.apache.commons.io."))) { + throw new ClassNotFoundException(name + " is hidden by this test"); + } + + return super.loadClass(name, resolve); + } + } + + private void expectAnyNumberOfDownloads() { + assertDoesNotThrow( + () -> expect(wagonManager.getWagon("file")).andReturn(wagon).anyTimes(), "This shouldn't happen!!"); + + expect(wagonManager.getAuthenticationInfo(anyString())).andReturn(null).anyTimes(); + expect(wagonManager.getProxy(anyString())).andReturn(null).anyTimes(); + + assertDoesNotThrow( + () -> { + wagon.connect( + anyObject(Repository.class), + anyObject(AuthenticationInfo.class), + anyObject(ProxyInfo.class)); + expectLastCall().anyTimes(); + + wagon.get(anyString(), anyObject(File.class)); + expectLastCall().anyTimes(); + + wagon.disconnect(); + expectLastCall().anyTimes(); + }, + "This shouldn't happen!!"); + + replay(wagon, wagonManager); + } + + private List listDownloadRoots() throws Exception { + Path tempRoot = Paths.get(System.getProperty("java.io.tmpdir")); + List roots = new ArrayList<>(); + + try (DirectoryStream stream = Files.newDirectoryStream(tempRoot, "maven-shared-io-downloads-*")) { + for (Path root : stream) { + roots.add(root); + } + } + + return roots; + } + + private void deleteRecursively(Path path) throws Exception { + try (Stream paths = Files.walk(path)) { + for (Path candidate : paths.sorted(Comparator.reverseOrder()).collect(Collectors.toList())) { + Files.deleteIfExists(candidate); + } + } + } + + private Set listDownloadTempFiles() throws Exception { + Path tempRoot = Paths.get(System.getProperty("java.io.tmpdir")); + Set files = new HashSet<>(); + + try (DirectoryStream roots = Files.newDirectoryStream(tempRoot, "maven-shared-io-downloads-*")) { + for (Path root : roots) { + try (Stream paths = Files.walk(root)) { + paths.filter(Files::isRegularFile).map(Path::toString).forEach(files::add); + } + } + } + + return files; + } + private void setupDefaultMockConfiguration() { assertDoesNotThrow( () -> {