Skip to content

Check that digesting consumes the expected number of bytes. #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ private static FileArtifactValue create(
return new DirectoryArtifactValue(path.getLastModifiedTime());
}
if (digest == null) {
digest = DigestUtils.getDigestWithManualFallback(path, xattrProvider);
digest = DigestUtils.getDigestWithManualFallback(path, size, xattrProvider);
}
Preconditions.checkState(digest != null, path);
return createForNormalFile(digest, proxy, size);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,11 @@ public BundledFileSystem() {

@Override
protected synchronized byte[] getFastDigest(PathFragment path) {
return getDigest(path);
return getDigest(path, -1);
}

@Override
protected synchronized byte[] getDigest(PathFragment path) {
protected synchronized byte[] getDigest(PathFragment path, long expectedSize) {
return getDigestFunction().getHashFunction().hashString(path.toString(), UTF_8).asBytes();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ private void updateRunfilesTree(
if (tree.getSymlinksMode() != SKIP
&& !outputManifest.isSymbolicLink()
&& Arrays.equals(
DigestUtils.getDigestWithManualFallback(outputManifest, xattrProvider),
DigestUtils.getDigestWithManualFallback(inputManifest, xattrProvider))
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(outputManifest, xattrProvider),
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(inputManifest, xattrProvider))
&& (OS.getCurrent() != OS.WINDOWS || isRunfilesDirectoryPopulated(runfilesDirPath))) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ protected Digest computeDigest(
// Try to obtain a digest from the filesystem.
return builder
.setHash(
HashCode.fromBytes(DigestUtils.getDigestWithManualFallback(path, xattrProvider))
HashCode.fromBytes(
DigestUtils.getDigestWithManualFallback(path, fileSize, xattrProvider))
.toString())
.setSizeBytes(fileSize)
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
}

@Override
protected byte[] getDigest(PathFragment path) throws IOException {
protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
path = resolveSymbolicLinks(path).asFragment();
// Try to obtain a fast digest through a stat. This is only possible for in-memory files.
// The parent path has already been canonicalized by resolveSymbolicLinks, so FOLLOW_NONE is
Expand All @@ -443,7 +443,7 @@ protected byte[] getDigest(PathFragment path) throws IOException {
if (status instanceof FileStatusWithDigest) {
return ((FileStatusWithDigest) status).getDigest();
}
return localFs.getPath(path).getDigest();
return localFs.getPath(path).getDigest(expectedSize);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ public Digest compute(Path file) throws IOException {
}

public Digest compute(Path file, long fileSize) throws IOException {
return buildDigest(DigestUtils.getDigestWithManualFallback(file, xattrProvider), fileSize);
return buildDigest(
DigestUtils.getDigestWithManualFallback(file, fileSize, xattrProvider), fileSize);
}

public Digest compute(VirtualActionInput input) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ private FileArtifactValue constructFileArtifactValue(
// possible to hit the digest cache - we probably already computed the digest for the
// target during previous action execution.
Path pathToDigest = isResolvedSymlink ? statAndValue.realPath() : statAndValue.pathNoFollow();
actualDigest = DigestUtils.manuallyComputeDigest(pathToDigest);
actualDigest = DigestUtils.manuallyComputeDigest(pathToDigest, value.getSize());
}

if (!isResolvedSymlink) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ public boolean createWritableDirectory(PathFragment path) throws IOException {
}

@Override
public byte[] getDigest(PathFragment path) throws IOException {
return super.getDigest(path);
public byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
return super.getDigest(path, expectedSize);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,11 +446,11 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
}

@Override
protected byte[] getDigest(PathFragment path) throws IOException {
protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
String name = path.toString();
long startTime = Profiler.nanoTimeMaybe();
try {
return super.getDigest(path);
return super.getDigest(path, expectedSize);
} finally {
profiler.logSimpleTask(startTime, ProfilerTask.VFS_MD5, name);
}
Expand Down
33 changes: 27 additions & 6 deletions src/main/java/com/google/devtools/build/lib/vfs/DigestUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -160,19 +160,40 @@ public static CacheStats getCacheStats() {
* #manuallyComputeDigest} to skip an additional attempt to obtain the fast digest.
*
* @param path Path of the file.
* @param fileSize Size of the file. Used to determine if digest calculation should be done
* serially or in parallel. Files larger than a certain threshold will be read serially, in
* order to avoid excessive disk seeks.
*/
public static byte[] getDigestWithManualFallback(Path path, XattrProvider xattrProvider)
throws IOException {
public static byte[] getDigestWithManualFallback(
Path path, long fileSize, XattrProvider xattrProvider) throws IOException {
byte[] digest = xattrProvider.getFastDigest(path);
return digest != null ? digest : manuallyComputeDigest(path);
return digest != null ? digest : manuallyComputeDigest(path, fileSize);
}

/**
* Gets the digest of {@code path}, using a constant-time xattr call if the filesystem supports
* it, and calculating the digest manually otherwise.
*
* <p>Unlike {@link #getDigestWithManualFallback}, will not rate-limit manual digesting of files,
* so only use this method if the file size is truly unknown and you don't expect many concurrent
* manual digests of large files.
*
* @param path Path of the file.
*/
public static byte[] getDigestWithManualFallbackWhenSizeUnknown(
Path path, XattrProvider xattrProvider) throws IOException {
return getDigestWithManualFallback(path, -1, xattrProvider);
}
Comment on lines +173 to 186

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider whether it would be better to have a single getDigestWithManualFallback method with an optional expectedSize parameter (e.g., defaulting to -1) instead of having two separate methods. This might improve maintainability and reduce code duplication.


/**
* Calculates the digest manually.
*
* @param path Path of the file.
* @param fileSize Size of the file. Used to determine if digest calculation should be done
* serially or in parallel. Files larger than a certain threshold will be read serially, in
* order to avoid excessive disk seeks.
*/
public static byte[] manuallyComputeDigest(Path path) throws IOException {
public static byte[] manuallyComputeDigest(Path path, long fileSize) throws IOException {
byte[] digest;

// Attempt a cache lookup if the cache is enabled.
Expand All @@ -186,9 +207,9 @@ public static byte[] manuallyComputeDigest(Path path) throws IOException {
}
}

digest = path.getDigest();
digest = path.getDigest(fileSize);

Preconditions.checkNotNull(digest, "Missing digest for %s", path);
Preconditions.checkNotNull(digest, "Missing digest for %s (size %s)", path, fileSize);
if (cache != null) {
cache.put(key, digest);
}
Expand Down
29 changes: 22 additions & 7 deletions src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
import com.google.common.hash.Funnels;
import com.google.common.io.ByteSource;
import com.google.common.io.CharStreams;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
Expand All @@ -33,6 +34,7 @@
import java.util.Collection;
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
import javax.annotation.Nullable;

/** This interface models a file system. */
Expand Down Expand Up @@ -347,13 +349,26 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
* @return a new byte array containing the file's digest
* @throws IOException if the digest could not be computed for any reason
*/
protected byte[] getDigest(PathFragment path) throws IOException {
return new ByteSource() {
@Override
public InputStream openStream() throws IOException {
return getInputStream(path);
}
}.hash(digestFunction.getHashFunction()).asBytes();
protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
var bs =
new ByteSource() {
@Override
public InputStream openStream() throws IOException {
return getInputStream(path);
}
};
var hasher = digestFunction.getHashFunction().newHasher();
var copied = bs.copyTo(Funnels.asOutputStream(hasher));
if (expectedSize != -1 && copied != expectedSize) {
throw new IOException(
String.format(
Locale.US,
"digesting %s saw %s bytes rather than the expected %s",
path,
copied,
expectedSize));
}
return hasher.hash().asBytes();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -420,11 +420,11 @@ public void setLastModifiedTime(PathFragment path, long newTime) throws IOExcept
}

@Override
protected byte[] getDigest(PathFragment path) throws IOException {
protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
String name = path.toString();
long startTime = Profiler.nanoTimeMaybe();
try {
return super.getDigest(path);
return super.getDigest(path, expectedSize);
} finally {
profiler.logSimpleTask(startTime, ProfilerTask.VFS_MD5, name);
}
Expand Down
24 changes: 21 additions & 3 deletions src/main/java/com/google/devtools/build/lib/vfs/Path.java
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,23 @@ public byte[] getFastDigest() throws IOException {
* @throws IOException if the digest could not be computed for any reason
*/
public byte[] getDigest() throws IOException {
return fileSystem.getDigest(asFragment());
return getDigest(-1);
}

/**
* Returns the digest of the file denoted by the current path, following symbolic links. Is not
* guaranteed to call {@link #getFastDigest} internally, even if a fast digest is likely
* available. Callers should prefer {@link DigestUtils#getDigestWithManualFallback} to this method
* unless they know that a fast digest is unavailable and do not need the other features
* (disk-read rate-limiting, global cache) that {@link DigestUtils} provides.
*
* @param expectedSize If not -1, the expected number of bytes to digest. If this does not match
* the number of bytes digested, an {@link IOException} may be raised.
* @return a new byte array containing the file's digest
* @throws IOException if the digest could not be computed for any reason
*/
public byte[] getDigest(long expectedSize) throws IOException {
return fileSystem.getDigest(asFragment(), expectedSize);
}

/**
Expand Down Expand Up @@ -716,7 +732,8 @@ public String getDirectoryDigest(XattrProvider xattrProvider) throws IOException
} else {
hasher.putChar('-');
}
hasher.putBytes(DigestUtils.getDigestWithManualFallback(path, xattrProvider));
hasher.putBytes(
DigestUtils.getDigestWithManualFallback(path, stat.getSize(), xattrProvider));
} else if (stat.isDirectory()) {
hasher.putChar('d').putUnencodedChars(path.getDirectoryDigest(xattrProvider));
} else if (stat.isSymbolicLink()) {
Expand All @@ -730,7 +747,8 @@ public String getDirectoryDigest(XattrProvider xattrProvider) throws IOException
} else {
hasher.putChar('-');
}
hasher.putBytes(DigestUtils.getDigestWithManualFallback(resolved, xattrProvider));
hasher.putBytes(
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(resolved, xattrProvider));
} else {
// link to a non-file: include the link itself in the hash
hasher.putChar('l').putUnencodedChars(link.toString());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,8 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
}

@Override
protected byte[] getDigest(PathFragment path) throws IOException {
return delegateFs.getDigest(toDelegatePath(path));
protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
return delegateFs.getDigest(toDelegatePath(path), expectedSize);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ protected byte[] getFastDigest(PathFragment path) throws IOException {
if (stubbedFastDigestErrors.containsKey(path)) {
throw stubbedFastDigestErrors.get(path);
}
return getDigest(path);
return getDigest(path, -1);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ protected InputStream getInputStream(PathFragment path) throws IOException {
}

@Override
protected byte[] getDigest(PathFragment path) throws IOException {
protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
byte[] override = digestOverrides.get(path.getPathString());
return override != null ? override : super.getDigest(path);
return override != null ? override : super.getDigest(path, expectedSize);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,11 @@ protected static final class FakeActionFileSystem extends DelegateFileSystem {

@Override
protected byte[] getFastDigest(PathFragment path) throws IOException {
return super.getDigest(path);
return super.getDigest(path, -1);
}

@Override
protected byte[] getDigest(PathFragment path) throws IOException {
protected byte[] getDigest(PathFragment path, long expectedSize) throws IOException {
throw new UnsupportedOperationException();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static java.util.Arrays.stream;
import static org.junit.Assert.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doReturn;
Expand Down Expand Up @@ -578,9 +579,9 @@ public void getDigest_fromInputArtifactData_forLocalArtifact() throws Exception
// Verify that we don't fall back to a slow digest.
reset(fs);
assertThat(actionFs.getFastDigest(path)).isEqualTo(getDigest("local contents"));
verify(fs, never()).getDigest(any());
verify(fs, never()).getDigest(any(), anyLong());

assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("local contents"));
assertThat(actionFs.getDigest(path, -1)).isEqualTo(getDigest("local contents"));
}

@Test
Expand All @@ -593,9 +594,9 @@ public void getDigest_fromInputArtifactData_forRemoteArtifact() throws Exception
// Verify that we don't fall back to a slow digest.
reset(fs);
assertThat(actionFs.getFastDigest(path)).isEqualTo(getDigest("remote contents"));
verify(fs, never()).getDigest(any());
verify(fs, never()).getDigest(any(), anyLong());

assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("remote contents"));
assertThat(actionFs.getDigest(path, -1)).isEqualTo(getDigest("remote contents"));
}

@Test
Expand All @@ -606,7 +607,7 @@ public void getDigest_fromRemoteOutputTree() throws Exception {
injectRemoteFile(actionFs, artifact.getPath().asFragment(), "remote contents");

assertThat(actionFs.getFastDigest(path)).isEqualTo(getDigest("remote contents"));
assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("remote contents"));
assertThat(actionFs.getDigest(path, -1)).isEqualTo(getDigest("remote contents"));
}

@Test
Expand All @@ -617,7 +618,7 @@ public void getDigest_fromLocalFilesystem() throws Exception {
writeLocalFile(actionFs, artifact.getPath().asFragment(), "local contents");

assertThat(actionFs.getFastDigest(path)).isNull();
assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("local contents"));
assertThat(actionFs.getDigest(path, -1)).isEqualTo(getDigest("local contents"));
}

@Test
Expand All @@ -627,7 +628,7 @@ public void getDigest_notFound() throws Exception {
PathFragment path = artifact.getPath().asFragment();

assertThrows(FileNotFoundException.class, () -> actionFs.getFastDigest(path));
assertThrows(FileNotFoundException.class, () -> actionFs.getDigest(path));
assertThrows(FileNotFoundException.class, () -> actionFs.getDigest(path, -1));
}

@Test
Expand All @@ -650,7 +651,7 @@ public void getDigest_followSymlinks(
assertThat(actionFs.getFastDigest(linkPath)).isEqualTo(getDigest("content"));
}

assertThat(actionFs.getDigest(linkPath)).isEqualTo(getDigest("content"));
assertThat(actionFs.getDigest(linkPath, -1)).isEqualTo(getDigest("content"));
}

@Test
Expand Down
Loading
Loading