From 46ae8a2d25bd9168630602bb39208e74b12660e1 Mon Sep 17 00:00:00 2001 From: Megan Ketelaar Date: Fri, 31 May 2024 13:54:07 -0400 Subject: [PATCH 1/2] Reimplement NPM caching to fix concurrency --- .../com/diffplug/spotless/npm/ShadowCopy.java | 96 +++++++++---------- .../diffplug/spotless/npm/ShadowCopyTest.java | 17 ---- 2 files changed, 47 insertions(+), 66 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/npm/ShadowCopy.java b/lib/src/main/java/com/diffplug/spotless/npm/ShadowCopy.java index 0241d0cbcc..43019f09d4 100644 --- a/lib/src/main/java/com/diffplug/spotless/npm/ShadowCopy.java +++ b/lib/src/main/java/com/diffplug/spotless/npm/ShadowCopy.java @@ -17,6 +17,8 @@ import java.io.File; import java.io.IOException; +import java.nio.file.AtomicMoveNotSupportedException; +import java.nio.file.DirectoryNotEmptyException; import java.nio.file.FileAlreadyExistsException; import java.nio.file.FileSystemException; import java.nio.file.FileVisitResult; @@ -24,9 +26,8 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.SimpleFileVisitor; +import java.nio.file.StandardCopyOption; import java.nio.file.attribute.BasicFileAttributes; -import java.time.Duration; -import java.util.concurrent.TimeoutException; import java.util.function.Supplier; import javax.annotation.Nonnull; @@ -55,19 +56,47 @@ private File shadowCopyRoot() { } public void addEntry(String key, File orig) { - // prevent concurrent adding of entry with same key - if (!reserveSubFolder(key)) { - logger.debug("Shadow copy entry already in progress: {}. Awaiting finalization.", key); + File target = entry(key, orig.getName()); + if (target.exists()) { + logger.debug("Shadow copy entry already exists, not overwriting: {}", key); + } else { try { - NpmResourceHelper.awaitFileDeleted(markerFilePath(key).toFile(), Duration.ofSeconds(120)); - } catch (TimeoutException e) { - throw new RuntimeException(e); + storeEntry(key, orig, target); + } catch (Throwable ex) { + // Log but don't fail + logger.warn("Unable to store cache entry for {}", key, ex); } } + } + + private void storeEntry(String key, File orig, File target) throws IOException { + // Create a temp directory in the same directory as target + Files.createDirectories(target.toPath().getParent()); + Path tempDirectory = Files.createTempDirectory(target.toPath().getParent(), key); + logger.debug("Will store entry {} to temporary directory {}, which is a sibling of the ultimate target {}", orig, tempDirectory, target); + try { - storeEntry(key, orig); + // Copy orig to temp dir + Files.walkFileTree(orig.toPath(), new CopyDirectoryRecursively(tempDirectory, orig.toPath())); + try { + logger.debug("Finished storing entry {}. Atomically moving temporary directory {} into final place {}", key, tempDirectory, target); + // Atomically rename the completed cache entry into place + Files.move(tempDirectory, target.toPath(), StandardCopyOption.ATOMIC_MOVE); + } catch (FileAlreadyExistsException | DirectoryNotEmptyException e) { + // Someone already beat us to it + logger.debug("Shadow copy entry now exists, not overwriting: {}", key); + } catch (AtomicMoveNotSupportedException e) { + logger.warn("The filesystem at {} does not support atomic moves. Spotless cannot safely cache on such a system due to race conditions. Caching has been skipped.", target.toPath().getParent(), e); + } } finally { - cleanupReservation(key); + // Best effort to clean up + if (Files.exists(tempDirectory)) { + try { + Files.walkFileTree(tempDirectory, new DeleteDirectoryRecursively()); + } catch (Throwable ex) { + logger.warn("Ignoring error while cleaning up temporary copy", ex); + } + } } } @@ -75,42 +104,10 @@ public File getEntry(String key, String fileName) { return entry(key, fileName); } - private void storeEntry(String key, File orig) { - File target = entry(key, orig.getName()); - if (target.exists()) { - logger.debug("Shadow copy entry already exists: {}", key); - // delete directory "target" recursively - // https://stackoverflow.com/questions/3775694/deleting-folder-from-java - ThrowingEx.run(() -> Files.walkFileTree(target.toPath(), new DeleteDirectoryRecursively())); - } - // copy directory "orig" to "target" using hard links if possible or a plain copy otherwise - ThrowingEx.run(() -> Files.walkFileTree(orig.toPath(), new CopyDirectoryRecursively(target, orig))); - } - - private void cleanupReservation(String key) { - ThrowingEx.run(() -> Files.delete(markerFilePath(key))); - } - - private Path markerFilePath(String key) { - return Paths.get(shadowCopyRoot().getAbsolutePath(), key + ".marker"); - } - private File entry(String key, String origName) { return Paths.get(shadowCopyRoot().getAbsolutePath(), key, origName).toFile(); } - private boolean reserveSubFolder(String key) { - // put a marker file named "key".marker in "shadowCopyRoot" to make sure no other process is using it or return false if it already exists - try { - Files.createFile(Paths.get(shadowCopyRoot().getAbsolutePath(), key + ".marker")); - return true; - } catch (FileAlreadyExistsException e) { - return false; - } catch (IOException e) { - throw new RuntimeException(e); - } - } - public File copyEntryInto(String key, String origName, File targetParentFolder) { File target = Paths.get(targetParentFolder.getAbsolutePath(), origName).toFile(); if (target.exists()) { @@ -118,7 +115,7 @@ public File copyEntryInto(String key, String origName, File targetParentFolder) ThrowingEx.run(() -> Files.walkFileTree(target.toPath(), new DeleteDirectoryRecursively())); } // copy directory "orig" to "target" using hard links if possible or a plain copy otherwise - ThrowingEx.run(() -> Files.walkFileTree(entry(key, origName).toPath(), new CopyDirectoryRecursively(target, entry(key, origName)))); + ThrowingEx.run(() -> Files.walkFileTree(entry(key, origName).toPath(), new CopyDirectoryRecursively(target.toPath(), entry(key, origName).toPath()))); return target; } @@ -127,12 +124,12 @@ public boolean entryExists(String key, String origName) { } private static class CopyDirectoryRecursively extends SimpleFileVisitor { - private final File target; - private final File orig; + private final Path target; + private final Path orig; private boolean tryHardLink = true; - public CopyDirectoryRecursively(File target, File orig) { + public CopyDirectoryRecursively(Path target, Path orig) { this.target = target; this.orig = orig; } @@ -140,7 +137,7 @@ public CopyDirectoryRecursively(File target, File orig) { @Override public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { // create directory on target - Files.createDirectories(target.toPath().resolve(orig.toPath().relativize(dir))); + Files.createDirectories(target.resolve(orig.relativize(dir))); return super.preVisitDirectory(dir, attrs); } @@ -149,7 +146,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO // first try to hardlink, if that fails, copy if (tryHardLink) { try { - Files.createLink(target.toPath().resolve(orig.toPath().relativize(file)), file); + Files.createLink(target.resolve(orig.relativize(file)), file); return super.visitFile(file, attrs); } catch (UnsupportedOperationException | SecurityException | FileSystemException e) { logger.debug("Shadow copy entry does not support hard links: {}. Switching to 'copy'.", file, e); @@ -160,11 +157,12 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO } } // copy file to target - Files.copy(file, target.toPath().resolve(orig.toPath().relativize(file))); + Files.copy(file, target.resolve(orig.relativize(file))); return super.visitFile(file, attrs); } } + // https://stackoverflow.com/questions/3775694/deleting-folder-from-java private static class DeleteDirectoryRecursively extends SimpleFileVisitor { @Override public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { diff --git a/testlib/src/test/java/com/diffplug/spotless/npm/ShadowCopyTest.java b/testlib/src/test/java/com/diffplug/spotless/npm/ShadowCopyTest.java index 297fa1d5bd..969095c2f0 100644 --- a/testlib/src/test/java/com/diffplug/spotless/npm/ShadowCopyTest.java +++ b/testlib/src/test/java/com/diffplug/spotless/npm/ShadowCopyTest.java @@ -96,23 +96,6 @@ void changingAFolderAfterAddingItDoesNotChangeTheShadowCopy() throws IOException Assertions.assertThat(shadowCopy.listFiles()[0].getName()).isNotEqualTo(folderWithRandomFile.listFiles()[0].getName()); } - @Test - void addingTheSameEntryTwiceResultsInSecondEntryBeingRetained() throws IOException { - File folderWithRandomFile = newFolderWithRandomFile(); - shadowCopy.addEntry("someEntry", folderWithRandomFile); - - // now change the orig - Files.delete(folderWithRandomFile.listFiles()[0].toPath()); - File newRandomFile = new File(folderWithRandomFile, "replacedFile.txt"); - writeRandomStringOfLengthToFile(newRandomFile, 100); - - // and then add the same entry with new content again and check that they now are the same again - shadowCopy.addEntry("someEntry", folderWithRandomFile); - File shadowCopyFile = shadowCopy.getEntry("someEntry", folderWithRandomFile.getName()); - Assertions.assertThat(shadowCopyFile.listFiles()).hasSize(folderWithRandomFile.listFiles().length); - assertAllFilesAreEqualButNotSameAbsolutePath(folderWithRandomFile, shadowCopyFile); - } - @Test void aFolderCanBeCopiedUsingShadowCopy() throws IOException { File folderWithRandomFile = newFolderWithRandomFile(); From 94e22de569815f6894cc39698b9b03a5f3fce2e2 Mon Sep 17 00:00:00 2001 From: Megan Ketelaar Date: Fri, 31 May 2024 14:37:55 -0400 Subject: [PATCH 2/2] Add CHANGES entries --- CHANGES.md | 1 + plugin-gradle/CHANGES.md | 1 + plugin-maven/CHANGES.md | 1 + 3 files changed, 3 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 04722f1c2b..8f54fef846 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -18,6 +18,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( * Correctly provide EditorConfig property types for Ktlint ([#2052](https://github.com/diffplug/spotless/issues/2052)) * Made ShadowCopy (`npmInstallCache`) more robust by re-creating the cache dir if it goes missing ([#1984](https://github.com/diffplug/spotless/issues/1984),[2096](https://github.com/diffplug/spotless/pull/2096)) * scalafmt.conf fileOverride section now works correctly ([#1854](https://github.com/diffplug/spotless/pull/1854)) +* Reworked ShadowCopy (`npmInstallCache`) to use atomic filesystem operations, resolving several race conditions that could arise ([#2151](https://github.com/diffplug/spotless/pull/2151)) ### Changes * Bump default `cleanthat` version to latest `2.16` -> `2.20`. ([#1725](https://github.com/diffplug/spotless/pull/1725)) * Bump default `gherkin-utils` version to latest `8.0.2` -> `9.0.0`. ([#1703](https://github.com/diffplug/spotless/pull/1703)) diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md index 99160a6011..6d09875fcd 100644 --- a/plugin-gradle/CHANGES.md +++ b/plugin-gradle/CHANGES.md @@ -15,6 +15,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( * Fixed memory leak introduced in 6.21.0 ([#2067](https://github.com/diffplug/spotless/issues/2067)) * Made ShadowCopy (`npmInstallCache`) more robust by re-creating the cache dir if it goes missing ([#1984](https://github.com/diffplug/spotless/issues/1984),[2096](https://github.com/diffplug/spotless/pull/2096)) * scalafmt.conf fileOverride section now works correctly ([#1854](https://github.com/diffplug/spotless/pull/1854)) +* Reworked ShadowCopy (`npmInstallCache`) to use atomic filesystem operations, resolving several race conditions that could arise ([#2151](https://github.com/diffplug/spotless/pull/2151)) ### Changes * Bump default `cleanthat` version to latest `2.16` -> `2.20`. ([#1725](https://github.com/diffplug/spotless/pull/1725)) * Bump default `gherkin-utils` version to latest `8.0.2` -> `9.0.0`. ([#1703](https://github.com/diffplug/spotless/pull/1703)) diff --git a/plugin-maven/CHANGES.md b/plugin-maven/CHANGES.md index f3ba81d5bd..8c67aaeed2 100644 --- a/plugin-maven/CHANGES.md +++ b/plugin-maven/CHANGES.md @@ -11,6 +11,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( * Correctly provide EditorConfig property types for Ktlint ([#2052](https://github.com/diffplug/spotless/issues/2052)) * Made ShadowCopy (`npmInstallCache`) more robust by re-creating the cache dir if it goes missing ([#1984](https://github.com/diffplug/spotless/issues/1984),[2096](https://github.com/diffplug/spotless/pull/2096)) * scalafmt.conf fileOverride section now works correctly ([#1854](https://github.com/diffplug/spotless/pull/1854)) +* Reworked ShadowCopy (`npmInstallCache`) to use atomic filesystem operations, resolving several race conditions that could arise ([#2151](https://github.com/diffplug/spotless/pull/2151)) ### Changes * Bump default `cleanthat` version to latest `2.16` -> `2.20`. ([#1725](https://github.com/diffplug/spotless/pull/1725)) * Bump default `gherkin-utils` version to latest `8.0.2` -> `9.0.0`. ([#1703](https://github.com/diffplug/spotless/pull/1703))