Skip to content

Commit

Permalink
Reimplement NPM caching to fix concurrency (#2151 tweaked by #2155)
Browse files Browse the repository at this point in the history
  • Loading branch information
@nedtwigg
nedtwigg authored Jun 4, 2024
2 parents 3308d33 + abf887d commit fe1c64a
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 66 deletions.
1 change: 1 addition & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,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))
Expand Down
99 changes: 50 additions & 49 deletions lib/src/main/java/com/diffplug/spotless/npm/ShadowCopy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,17 @@

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;
import java.nio.file.Files;
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;
Expand All @@ -36,6 +37,8 @@

import com.diffplug.spotless.ThrowingEx;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

class ShadowCopy {

private static final Logger logger = LoggerFactory.getLogger(ShadowCopy.class);
Expand All @@ -55,70 +58,67 @@ 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);
}
}
}

@SuppressFBWarnings("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE")
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);
}
}
}
}

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()) {
logger.warn("Shadow copy destination already exists, deleting! {}: {}", key, target);
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;
}

Expand All @@ -127,20 +127,20 @@ public boolean entryExists(String key, String origName) {
}

private static class CopyDirectoryRecursively extends SimpleFileVisitor<Path> {
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;
}

@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);
}

Expand All @@ -149,7 +149,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);
Expand All @@ -160,11 +160,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<Path> {
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
Expand Down
1 change: 1 addition & 0 deletions plugin-gradle/CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,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))
Expand Down
1 change: 1 addition & 0 deletions plugin-maven/CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,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))
Expand Down
17 changes: 0 additions & 17 deletions testlib/src/test/java/com/diffplug/spotless/npm/ShadowCopyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit fe1c64a

Please sign in to comment.