diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCache.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCache.java index 07bc07165521d2..1c350ea7a71053 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCache.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCache.java @@ -26,12 +26,14 @@ import com.google.devtools.build.lib.vfs.Path; import java.io.IOException; import java.io.InputStream; -import java.io.OutputStream; import java.util.UUID; import javax.annotation.Nullable; -/** The cache implementation to store download artifacts from external repositories. - * TODO(jingwen): Implement file locking for concurrent cache accesses. +/** + * The cache implementation to store download artifacts from external repositories. + * + *

Operations performed by this class are atomic on the file system level under the assumption + * that the cache directory is not subject to concurrent file deletion. */ public class RepositoryCache { @@ -134,11 +136,6 @@ boolean hasCanonicalId(String cacheKey, KeyType keyType, String canonicalId) { .exists(); } - public synchronized Path get(String cacheKey, Path targetPath, KeyType keyType) - throws IOException, InterruptedException { - return get(cacheKey, targetPath, keyType, null); - } - /** * Copy or hardlink cached value to a specified directory, if it exists. * @@ -156,12 +153,8 @@ public synchronized Path get(String cacheKey, Path targetPath, KeyType keyType) * @throws IOException */ @Nullable - public synchronized Path get( - String cacheKey, Path targetPath, KeyType keyType, String canonicalId) + public Path get(String cacheKey, Path targetPath, KeyType keyType, String canonicalId) throws IOException, InterruptedException { - if (Thread.interrupted()) { - throw new InterruptedException(); - } Preconditions.checkState(isEnabled()); assertKeyIsValid(cacheKey, keyType); @@ -202,11 +195,6 @@ public synchronized Path get( return targetPath; } - public synchronized void put(String cacheKey, Path sourcePath, KeyType keyType) - throws IOException, InterruptedException { - put(cacheKey, sourcePath, keyType, null); - } - /** * Copies a value from a specified path into the cache. * @@ -217,13 +205,8 @@ public synchronized void put(String cacheKey, Path sourcePath, KeyType keyType) * restricted cache lookups later. * @throws IOException */ - public synchronized void put( - String cacheKey, Path sourcePath, KeyType keyType, String canonicalId) - throws IOException, InterruptedException { - // Check for interrupts while waiting for the monitor of this synchronized method - if (Thread.interrupted()) { - throw new InterruptedException(); - } + public void put(String cacheKey, Path sourcePath, KeyType keyType, String canonicalId) + throws IOException { Preconditions.checkState(isEnabled()); assertKeyIsValid(cacheKey, keyType); @@ -237,19 +220,11 @@ public synchronized void put( FileSystemUtils.moveFile(tmpName, cacheValue); if (!Strings.isNullOrEmpty(canonicalId)) { - byte[] canonicalIdBytes = canonicalId.getBytes(UTF_8); - String idHash = keyType.newHasher().putBytes(canonicalIdBytes).hash().toString(); - OutputStream idStream = cacheEntry.getRelative(ID_PREFIX + idHash).getOutputStream(); - idStream.write(canonicalIdBytes); - idStream.close(); + String idHash = keyType.newHasher().putBytes(canonicalId.getBytes(UTF_8)).hash().toString(); + FileSystemUtils.touchFile(cacheEntry.getRelative(ID_PREFIX + idHash)); } } - public synchronized String put(Path sourcePath, KeyType keyType) - throws IOException, InterruptedException { - return put(sourcePath, keyType, null); - } - /** * Copies a value from a specified path into the cache, computing the cache key itself. * @@ -260,7 +235,7 @@ public synchronized String put(Path sourcePath, KeyType keyType) * @throws IOException * @return The key for the cached entry. */ - public synchronized String put(Path sourcePath, KeyType keyType, String canonicalId) + public String put(Path sourcePath, KeyType keyType, String canonicalId) throws IOException, InterruptedException { String cacheKey = getChecksum(keyType, sourcePath); put(cacheKey, sourcePath, keyType, canonicalId); diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCacheTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCacheTest.java index 2b0f2e5c142932..e4a9104a0fefee 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/cache/RepositoryCacheTest.java @@ -74,7 +74,8 @@ public void testNonExistentCacheValue() { /** Test that the put method correctly stores the downloaded file into the cache. */ @Test public void testPutCacheValue() throws Exception { - repositoryCache.put(downloadedFileSha256, downloadedFile, KeyType.SHA256); + repositoryCache.put( + downloadedFileSha256, downloadedFile, KeyType.SHA256, /* canonicalId= */ null); Path cacheEntry = KeyType.SHA256.getCachePath(contentAddressableCachePath).getChild(downloadedFileSha256); Path cacheValue = cacheEntry.getChild(RepositoryCache.DEFAULT_CACHE_FILENAME); @@ -88,7 +89,7 @@ public void testPutCacheValue() throws Exception { */ @Test public void testPutCacheValueWithoutHash() throws Exception { - String cacheKey = repositoryCache.put(downloadedFile, KeyType.SHA256); + String cacheKey = repositoryCache.put(downloadedFile, KeyType.SHA256, /* canonicalId= */ null); assertThat(cacheKey).isEqualTo(downloadedFileSha256); Path cacheEntry = @@ -105,8 +106,10 @@ public void testPutCacheValueWithoutHash() throws Exception { */ @Test public void testPutCacheValueIdempotent() throws Exception { - repositoryCache.put(downloadedFileSha256, downloadedFile, KeyType.SHA256); - repositoryCache.put(downloadedFileSha256, downloadedFile, KeyType.SHA256); + repositoryCache.put( + downloadedFileSha256, downloadedFile, KeyType.SHA256, /* canonicalId= */ null); + repositoryCache.put( + downloadedFileSha256, downloadedFile, KeyType.SHA256, /* canonicalId= */ null); Path cacheEntry = KeyType.SHA256.getCachePath(contentAddressableCachePath).getChild(downloadedFileSha256); Path cacheValue = cacheEntry.getChild(RepositoryCache.DEFAULT_CACHE_FILENAME); @@ -119,11 +122,14 @@ public void testPutCacheValueIdempotent() throws Exception { @Test public void testGetCacheValue() throws Exception { // Inject file into cache - repositoryCache.put(downloadedFileSha256, downloadedFile, KeyType.SHA256); + repositoryCache.put( + downloadedFileSha256, downloadedFile, KeyType.SHA256, /* canonicalId= */ null); Path targetDirectory = scratch.dir("/external"); Path targetPath = targetDirectory.getChild(downloadedFile.getBaseName()); - Path actualTargetPath = repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256); + Path actualTargetPath = + repositoryCache.get( + downloadedFileSha256, targetPath, KeyType.SHA256, /* canonicalId= */ null); // Check that the contents are the same. assertThat(FileSystemUtils.readContent(downloadedFile, Charset.defaultCharset())) @@ -138,7 +144,9 @@ public void testGetCacheValue() throws Exception { public void testGetNullCacheValue() throws Exception { Path targetDirectory = scratch.dir("/external"); Path targetPath = targetDirectory.getChild(downloadedFile.getBaseName()); - Path actualTargetPath = repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256); + Path actualTargetPath = + repositoryCache.get( + downloadedFileSha256, targetPath, KeyType.SHA256, /* canonicalId= */ null); assertThat(actualTargetPath).isNull(); } @@ -148,7 +156,7 @@ public void testInvalidSha256Throws() throws Exception { String invalidSha = "foo"; thrown.expect(IOException.class); thrown.expectMessage("Invalid key \"foo\" of type SHA-256"); - repositoryCache.put(invalidSha, downloadedFile, KeyType.SHA256); + repositoryCache.put(invalidSha, downloadedFile, KeyType.SHA256, /* canonicalId= */ null); } @Test @@ -165,7 +173,7 @@ public void testPoisonedCache() throws Exception { thrown.expectMessage("does not match expected"); thrown.expectMessage("Please delete the directory"); - repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256); + repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256, /* canonicalId= */ null); } @Test @@ -215,7 +223,9 @@ public void testCanonicalId() throws Exception { repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256, "barid"); assertThat(lookupOtherId).isNull(); - Path lookupNoId = repositoryCache.get(downloadedFileSha256, targetPath, KeyType.SHA256); + Path lookupNoId = + repositoryCache.get( + downloadedFileSha256, targetPath, KeyType.SHA256, /* canonicalId= */ null); assertThat(lookupNoId).isEqualTo(targetPath); } }