diff --git a/maven-resolver-impl/pom.xml b/maven-resolver-impl/pom.xml index 6a0cfd406..60950c0d2 100644 --- a/maven-resolver-impl/pom.xml +++ b/maven-resolver-impl/pom.xml @@ -96,6 +96,11 @@ maven-resolver-test-util test + + com.google.jimfs + jimfs + test + diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java index 53f02d9ab..28b3b0fc9 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java @@ -31,6 +31,7 @@ import java.nio.channels.FileLock; import java.nio.channels.OverlappingFileLockException; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.util.Map; @@ -61,15 +62,14 @@ public Properties read(File file) { @Override public Properties read(Path path) { if (Files.isReadable(path)) { - synchronized (getMutex(path)) { - try { - long fileSize = Files.size(path); - try (FileChannel fileChannel = FileChannel.open(path, StandardOpenOption.READ); - FileLock unused = fileLock(fileChannel, Math.max(1, fileSize), true)) { - Properties props = new Properties(); - props.load(Channels.newInputStream(fileChannel)); - return props; - } + synchronized (mutex(path)) { + try (FileChannel fileChannel = FileChannel.open(path, StandardOpenOption.READ); + FileLock unused = fileLock(fileChannel, true)) { + Properties props = new Properties(); + props.load(Channels.newInputStream(fileChannel)); + return props; + } catch (NoSuchFileException e) { + LOGGER.debug("No such file to read {}: {}", path, e.getMessage()); } catch (IOException e) { LOGGER.warn("Failed to read tracking file '{}'", path, e); throw new UncheckedIOException(e); @@ -87,72 +87,79 @@ public Properties update(File file, Map updates) { @Override public Properties update(Path path, Map updates) { - Properties props = new Properties(); try { Files.createDirectories(path.getParent()); } catch (IOException e) { LOGGER.warn("Failed to create tracking file parent '{}'", path, e); throw new UncheckedIOException(e); } - - synchronized (getMutex(path)) { - try { - long fileSize; - try { - fileSize = Files.size(path); - } catch (IOException e) { - fileSize = 0L; + synchronized (mutex(path)) { + try (FileChannel fileChannel = FileChannel.open( + path, StandardOpenOption.READ, StandardOpenOption.WRITE, StandardOpenOption.CREATE); + FileLock unused = fileLock(fileChannel, false)) { + Properties props = new Properties(); + if (fileChannel.size() > 0) { + props.load(Channels.newInputStream(fileChannel)); } - try (FileChannel fileChannel = FileChannel.open( - path, StandardOpenOption.READ, StandardOpenOption.WRITE, StandardOpenOption.CREATE); - FileLock unused = fileLock(fileChannel, Math.max(1, fileSize), false)) { - if (fileSize > 0) { - props.load(Channels.newInputStream(fileChannel)); - } - for (Map.Entry update : updates.entrySet()) { - if (update.getValue() == null) { - props.remove(update.getKey()); - } else { - props.setProperty(update.getKey(), update.getValue()); - } + for (Map.Entry update : updates.entrySet()) { + if (update.getValue() == null) { + props.remove(update.getKey()); + } else { + props.setProperty(update.getKey(), update.getValue()); } - - LOGGER.debug("Writing tracking file '{}'", path); - ByteArrayOutputStream stream = new ByteArrayOutputStream(1024 * 2); - props.store( - stream, - "NOTE: This is a Maven Resolver internal implementation file" - + ", its format can be changed without prior notice."); - fileChannel.position(0); - int written = fileChannel.write(ByteBuffer.wrap(stream.toByteArray())); - fileChannel.truncate(written); } + + LOGGER.debug("Writing tracking file '{}'", path); + ByteArrayOutputStream stream = new ByteArrayOutputStream(1024 * 2); + props.store( + stream, + "NOTE: This is a Maven Resolver internal implementation file" + + ", its format can be changed without prior notice."); + fileChannel.position(0); + int written = fileChannel.write(ByteBuffer.wrap(stream.toByteArray())); + fileChannel.truncate(written); + return props; } catch (IOException e) { LOGGER.warn("Failed to write tracking file '{}'", path, e); throw new UncheckedIOException(e); } } + } - return props; + @Override + public boolean delete(File file) { + return delete(file.toPath()); + } + + @Override + public boolean delete(Path path) { + if (Files.isReadable(path)) { + synchronized (mutex(path)) { + try (FileChannel fileChannel = FileChannel.open(path, StandardOpenOption.WRITE); + FileLock unused = fileLock(fileChannel, false)) { + Files.delete(path); + return true; + } catch (NoSuchFileException e) { + LOGGER.debug("No such file to delete {}: {}", path, e.getMessage()); + } catch (IOException e) { + LOGGER.warn("Failed to delete tracking file '{}'", path, e); + throw new UncheckedIOException(e); + } + } + } + return false; } - private Object getMutex(Path path) { - // The interned string of path is (mis)used as mutex, to exclude different threads going for same file, - // as JVM file locking happens on JVM not on Thread level. This is how original code did it ¯\_(ツ)_/¯ - /* - * NOTE: Locks held by one JVM must not overlap and using the canonical path is our best bet, still another - * piece of code might have locked the same file (unlikely though) or the canonical path fails to capture file - * identity sufficiently as is the case with Java 1.6 and symlinks on Windows. - */ + private Object mutex(Path path) { return path.toAbsolutePath().normalize().toString().intern(); } - private FileLock fileLock(FileChannel channel, long size, boolean shared) throws IOException { + private FileLock fileLock(FileChannel channel, boolean shared) throws IOException { FileLock lock = null; for (int attempts = 8; attempts >= 0; attempts--) { try { - lock = channel.lock(0, size, shared); + lock = channel.lock(0, Long.MAX_VALUE, shared); break; } catch (OverlappingFileLockException e) { if (attempts <= 0) { diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultUpdateCheckManager.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultUpdateCheckManager.java index 3d6133935..02e0da8f5 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultUpdateCheckManager.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultUpdateCheckManager.java @@ -22,8 +22,6 @@ import javax.inject.Named; import javax.inject.Singleton; -import java.io.IOException; -import java.io.UncheckedIOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.Collections; @@ -482,11 +480,7 @@ public void touchArtifact(RepositorySystemSession session, UpdateCheck updates); + + /** + * Deletes the specified properties file, if exists. If file existed and was deleted, returns {@code true}. + * + * @deprecated Use {@link #delete(Path)} instead. + * @since 1.9.25 + */ + @Deprecated + boolean delete(File file); + + /** + * Deletes the specified properties file, if exists. If file existed and was deleted, returns {@code true}. + * + * @since 2.0.14 + */ + boolean delete(Path path); } diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManagerTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManagerTest.java index dfa5094db..c58606335 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManagerTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManagerTest.java @@ -18,29 +18,83 @@ */ package org.eclipse.aether.internal.impl; -import java.io.File; +import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.nio.file.FileSystem; +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.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Properties; -import org.eclipse.aether.internal.test.util.TestFileUtils; -import org.junit.jupiter.api.Test; +import com.google.common.jimfs.Configuration; +import com.google.common.jimfs.Jimfs; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; import static org.junit.jupiter.api.Assertions.*; -/** - */ public class DefaultTrackingFileManagerTest { + public enum FS { + DEFAULT, + JIMFS_UNIX, + JIMFS_WINDOWS; + + @Override + public String toString() { + return super.name().toLowerCase(Locale.ENGLISH); + } + } + + private FileSystem fileSystem; + + private Path createTmpFile(FS fs) throws IOException { + return createTmpFile(fs, 1); + } + + private Path createTmpFile(FS fs, int iterations) throws IOException { + byte[] payload = "#COMMENT\nkey1=value1\nkey2 : value2\n".getBytes(StandardCharsets.UTF_8); + if (iterations > 1) { + int payloadLength = payload.length; + byte[] newPayload = new byte[payloadLength * iterations]; + for (int i = 0; i < iterations; i++) { + System.arraycopy(payload, 0, newPayload, i * payloadLength, payloadLength); + } + payload = newPayload; + } + Path tempFile; + if (fs == FS.DEFAULT) { + tempFile = Files.createTempFile(getClass().getSimpleName(), ".tmp"); + } else if (fs == FS.JIMFS_UNIX || fs == FS.JIMFS_WINDOWS) { + fileSystem = Jimfs.newFileSystem(fs == FS.JIMFS_WINDOWS ? Configuration.windows() : Configuration.unix()); + tempFile = fileSystem.getPath("tmp/file.properties"); + } else { + throw new IllegalStateException("unsupported FS: " + fs); + } + Files.createDirectories(tempFile.getParent()); + Files.write(tempFile, payload); + return tempFile; + } - @Test - void testRead() throws Exception { + @AfterEach + void cleanup() throws IOException { + if (fileSystem != null) { + fileSystem.close(); + } + } + + @ParameterizedTest + @EnumSource(FS.class) + void testRead(FS fs) throws Exception { TrackingFileManager tfm = new DefaultTrackingFileManager(); - File propFile = TestFileUtils.createTempFile("#COMMENT\nkey1=value1\nkey2 : value2"); + Path propFile = createTmpFile(fs); Properties props = tfm.read(propFile); assertNotNull(props); @@ -48,30 +102,31 @@ void testRead() throws Exception { assertEquals("value1", props.get("key1")); assertEquals("value2", props.get("key2")); - assertTrue(propFile.delete(), "Leaked file: " + propFile); + assertDoesNotThrow(() -> Files.delete(propFile), "Leaked file" + propFile); props = tfm.read(propFile); assertNull(props, String.valueOf(props)); } - @Test - void testReadNoFileLeak() throws Exception { + @ParameterizedTest + @EnumSource(FS.class) + void testReadNoFileLeak(FS fs) throws Exception { TrackingFileManager tfm = new DefaultTrackingFileManager(); for (int i = 0; i < 1000; i++) { - File propFile = TestFileUtils.createTempFile("#COMMENT\nkey1=value1\nkey2 : value2"); + Path propFile = createTmpFile(fs); assertNotNull(tfm.read(propFile)); - assertTrue(propFile.delete(), "Leaked file: " + propFile); + assertDoesNotThrow(() -> Files.delete(propFile), "Leaked file" + propFile); } } - @Test - void testUpdate() throws Exception { + @ParameterizedTest + @EnumSource(FS.class) + void testUpdate(FS fs) throws Exception { TrackingFileManager tfm = new DefaultTrackingFileManager(); // NOTE: The excessive repetitions are to check the update properly truncates the file - File propFile = - TestFileUtils.createTempFile("key1=value1\nkey2 : value2\n".getBytes(StandardCharsets.UTF_8), 1000); + Path propFile = createTmpFile(fs, 1000); Map updates = new HashMap<>(); updates.put("key1", "v"); @@ -87,36 +142,50 @@ void testUpdate() throws Exception { assertNull(props.get("key2"), String.valueOf(props.get("key2"))); } - @Test - void testUpdateNoFileLeak() throws Exception { + @ParameterizedTest + @EnumSource(FS.class) + void testUpdateNoFileLeak(FS fs) throws Exception { TrackingFileManager tfm = new DefaultTrackingFileManager(); Map updates = new HashMap<>(); updates.put("k", "v"); for (int i = 0; i < 1000; i++) { - File propFile = TestFileUtils.createTempFile("#COMMENT\nkey1=value1\nkey2 : value2"); + Path propFile = createTmpFile(fs); assertNotNull(tfm.update(propFile, updates)); - assertTrue(propFile.delete(), "Leaked file: " + propFile); + assertDoesNotThrow(() -> Files.delete(propFile), "Leaked file" + propFile); + } + } + + @ParameterizedTest + @EnumSource(FS.class) + public void testDeleteFileIsGone(FS fs) throws Exception { + TrackingFileManager tfm = new DefaultTrackingFileManager(); + + for (int i = 0; i < 1000; i++) { + Path propFile = createTmpFile(fs); + assertTrue(tfm.delete(propFile)); + assertFalse(Files.exists(propFile), "File is not gone"); } } - @Test - void testLockingOnCanonicalPath() throws Exception { + @ParameterizedTest + @EnumSource(FS.class) + void testLockingOnCanonicalPath(FS fs) throws Exception { final TrackingFileManager tfm = new DefaultTrackingFileManager(); - final File propFile = TestFileUtils.createTempFile("#COMMENT\nkey1=value1\nkey2 : value2"); + Path propFile = createTmpFile(fs); final List errors = Collections.synchronizedList(new ArrayList<>()); Thread[] threads = new Thread[4]; for (int i = 0; i < threads.length; i++) { - String path = propFile.getParent(); + String path = propFile.getParent().toString(); for (int j = 0; j < i; j++) { path += "/."; } - path += "/" + propFile.getName(); - final File file = new File(path); + path += "/" + propFile.getFileName(); + final Path file = fileSystem != null ? fileSystem.getPath(path) : Paths.get(path); threads[i] = new Thread(() -> { try {