Skip to content

Commit

Permalink
Implement a native file OutputStream for Unix.
Browse files Browse the repository at this point in the history
Bazel's VFS classes make the assumption that all filenames are encoded with latin-1. That theoretically allows roundtripping any sort of horrible byte pattern a Unix filesystem can produce through Bazel's Path class. This scheme falls apart, though, when trying to use the JDK I/O libraries. The filename encoding assumed by the JDK I/O libraries comes from the sun.jnu.encoding property, which can't be overriden with the normal -D JVM command line syntax. The Bazel client still tries quite hard to force this property to be latin-1: https://github.com/bazelbuild/bazel/blob/6641ad986f436926a75b31b47314c193a9a7e032/src/main/cpp/blaze.cc#L1467-L1473 But even a fusillade of 4 environmental variables is sometimes not enough. On macOS, the JDK simply hardcodes UTF-8 as sun.jnu.encoding. Even on Linux, if a the en_US.ISO-8859-1 locale isn't installed, glibc will fall back to an ASCII encoding. Since there's no public way to create a JDK FileOutputStream from either a byte[] filename or a raw file descriptor, I conclude the only workaround is to implement open() and write() in Bazel's unix_jni. This CL does that.

We should probably implement a native file InputStream, too, for completeness. However, as merely implementing OutputStream fixes the relevant issue, I'm only doing that in this CL.

Fixes bazelbuild#7055.
  • Loading branch information
benjaminp committed Mar 19, 2019
1 parent 6641ad9 commit bb6e1af
Show file tree
Hide file tree
Showing 6 changed files with 191 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
import static com.google.devtools.build.lib.bazel.repository.StripPrefixedPath.maybeDeprefixSymlink;

import com.google.common.base.Optional;
import com.google.common.io.ByteStreams;
import com.google.devtools.build.lib.bazel.repository.DecompressorValue.Decompressor;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.StandardCopyOption;
import java.io.OutputStream;
import java.util.Date;
import java.util.HashSet;
import java.util.Set;
Expand Down Expand Up @@ -81,8 +81,9 @@ public Path decompress(DecompressorDescriptor descriptor) throws IOException {
filename, descriptor.repositoryPath().getRelative(linkName));
}
} else {
Files.copy(
tarStream, filename.getPathFile().toPath(), StandardCopyOption.REPLACE_EXISTING);
try (OutputStream out = filename.getOutputStream()) {
ByteStreams.copy(tarStream, out);
}
filename.chmod(entry.getMode());

// This can only be done on real files, not links, or it will skip the reader to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,17 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import com.google.common.io.ByteStreams;
import com.google.devtools.build.lib.bazel.repository.DecompressorValue.Decompressor;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.zip.ZipFileEntry;
import com.google.devtools.build.zip.ZipReader;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.StandardCopyOption;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -154,13 +153,9 @@ private static void extractZipEntry(
target = maybeDeprefixSymlink(target, prefix, destinationDirectory);
symlinks.put(outputPath, target);
} else {
// TODO(kchodorow): should be able to be removed when issue #236 is resolved, but for now
// this delete+rewrite is required or the build will error out if outputPath exists here.
// The zip file is not re-unzipped when the WORKSPACE file is changed (because it is assumed
// to be immutable) but is on server restart (which is a bug).
File outputFile = outputPath.getPathFile();
try (InputStream input = reader.getInputStream(entry)) {
Files.copy(input, outputFile.toPath(), StandardCopyOption.REPLACE_EXISTING);
try (InputStream input = reader.getInputStream(entry);
OutputStream output = outputPath.getOutputStream()) {
ByteStreams.copy(input, output);
}
outputPath.chmod(permissions);
outputPath.setLastModifiedTime(entry.getTime());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -467,4 +467,21 @@ public static void rmTree(String path) throws IOException {
}
remove(path.toString());
}

/**
* Open a file descriptor for writing.
*
* <p>This is a low level API. The caller is responsible for calling {@link close} on the returned
* file descriptor.
*
* @param path file to open
* @param append whether to open is append mode
*/
public static native int openWrite(String path, boolean append) throws FileNotFoundException;

/** Write a segment of data to a file descriptor. */
public static native int write(int fd, byte[] data, int off, int len) throws IOException;

/** Close a file descriptor. */
public static native int close(int fd) throws IOException;
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.OutputStream;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
Expand Down Expand Up @@ -419,4 +421,73 @@ protected void createFSDependentHardLink(Path linkPath, Path originalPath)
throws IOException {
NativePosixFiles.link(originalPath.toString(), linkPath.toString());
}

@Override
protected OutputStream createFileOutputStream(Path path, boolean append)
throws FileNotFoundException {
final String name = path.toString();
if (profiler.isActive()
&& (profiler.isProfiling(ProfilerTask.VFS_WRITE)
|| profiler.isProfiling(ProfilerTask.VFS_OPEN))) {
long startTime = Profiler.nanoTimeMaybe();
try {
return new ProfiledNativeFileOutputStream(NativePosixFiles.openWrite(name, append), name);
} finally {
profiler.logSimpleTask(startTime, ProfilerTask.VFS_OPEN, name);
}
} else {
return new NativeFileOutputStream(NativePosixFiles.openWrite(name, append));
}
}

private static class NativeFileOutputStream extends OutputStream {
private final int fd;
private boolean closed = false;

NativeFileOutputStream(int fd) {
this.fd = fd;
}

@Override
public void close() throws IOException {
if (!closed) {
closed = true;
NativePosixFiles.close(fd);
}
}

@Override
public void write(int b) throws IOException {
write(new byte[] {(byte) (b & 0xFF)});
}

@Override
public void write(byte b[]) throws IOException {
write(b, 0, b.length);
}

@Override
public void write(byte b[], int off, int len) throws IOException {
NativePosixFiles.write(fd, b, off, len);
}
}

private static final class ProfiledNativeFileOutputStream extends NativeFileOutputStream {
private final String name;

public ProfiledNativeFileOutputStream(int fd, String name) throws FileNotFoundException {
super(fd);
this.name = name;
}

@Override
public void write(byte[] b, int off, int len) throws IOException {
long startTime = Profiler.nanoTimeMaybe();
try {
super.write(b, off, len);
} finally {
profiler.logSimpleTask(startTime, ProfilerTask.VFS_WRITE, name);
}
}
}
}
66 changes: 66 additions & 0 deletions src/main/native/unix_jni.cc
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,72 @@ Java_com_google_devtools_build_lib_unix_NativePosixFiles_lgetxattr(JNIEnv *env,
return ::getxattr_common(env, path, name, ::portable_lgetxattr);
}

extern "C" JNIEXPORT jint JNICALL
Java_com_google_devtools_build_lib_unix_NativePosixFiles_openWrite(JNIEnv *env,
jclass clazz,
jstring path,
jboolean append) {
const char *path_chars = GetStringLatin1Chars(env, path);
int flags = (O_WRONLY | O_CREAT) | (append ? O_APPEND : O_TRUNC);
int fd;
while ((fd = open(path_chars, flags, 0666)) == -1 && errno == EINTR) { }
if (fd == -1) {
// The interface only allows FileNotFoundException.
::PostException(env, ENOENT,
std::string(path_chars) + " (" + ErrorMessage(errno)
+ ")");
}
ReleaseStringLatin1Chars(path_chars);
return fd;
}

extern "C" JNIEXPORT void JNICALL
Java_com_google_devtools_build_lib_unix_NativePosixFiles_close(JNIEnv *env,
jclass clazz,
jint fd) {
if (close(fd) == -1) {
::PostException(env, errno, "error when closing file");
}
}

extern "C" JNIEXPORT void JNICALL
Java_com_google_devtools_build_lib_unix_NativePosixFiles_write(JNIEnv *env,
jclass clazz,
jint fd,
jbyteArray data,
jint off,
jint len) {
int data_len = env->GetArrayLength(data);
if (off < 0 || len < 0 || off > data_len || data_len - off < len) {
jclass oob = env->FindClass("java/lang/IndexOutOfBoundsException");
if (oob != nullptr) {
env->ThrowNew(oob, nullptr);
}
return;
}
jbyte *buf = static_cast<jbyte *>(malloc(len));
if (buf == nullptr) {
::PostException(env, ENOMEM, "out of memory");
return;
}
env->GetByteArrayRegion(data, off, len, buf);
if (!env->ExceptionOccurred()) {
jbyte *p = buf;
while (len > 0) {
ssize_t res = write(fd, p, len);
if (res == -1) {
if (errno != EINTR) {
::PostException(env, errno, "writing file failed");
}
} else {
p += res;
len -= res;
}
}
}
free(buf);
}


// Computes MD5 digest of "file", writes result in "result", which
// must be of length Md5Digest::kDigestLength. Returns zero on success, or
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/**
* This class tests the FilesystemUtils class.
*/
/** Tests for the {@link NativePosixFiles} class. */
@RunWith(JUnit4.class)
public class NativePosixFilesTest {
private FileSystem testFS;
Expand Down Expand Up @@ -156,4 +154,30 @@ public void testGetxattr_FileNotFound() throws Exception {
assertThrows(
FileNotFoundException.class, () -> NativePosixFiles.lgetxattr(nonexistentFile, "foo"));
}

@Test
public void writing() throws Exception {
java.nio.file.Path myfile = Files.createTempFile("myfile", null);
int fd1 = NativePosixFiles.openWrite(myfile.toString(), false);
assertThrows(
IndexOutOfBoundsException.class,
() -> NativePosixFiles.write(fd1, new byte[] {0, 1, 2, 3}, 5, 1));
assertThrows(
IndexOutOfBoundsException.class,
() -> NativePosixFiles.write(fd1, new byte[] {0, 1, 2, 3}, -1, 1));
assertThrows(
IndexOutOfBoundsException.class,
() -> NativePosixFiles.write(fd1, new byte[] {0, 1, 2, 3}, 0, -1));
assertThrows(
IndexOutOfBoundsException.class,
() -> NativePosixFiles.write(fd1, new byte[] {0, 1, 2, 3}, 0, 5));
NativePosixFiles.write(fd1, new byte[] {0, 1, 2, 3}, 0, 4);
NativePosixFiles.close(fd1);
assertThat(Files.readAllBytes(myfile)).isEqualTo(new byte[] {0, 1, 2, 3});
// Try appending.
int fd2 = NativePosixFiles.openWrite(myfile.toString(), true);
NativePosixFiles.write(fd2, new byte[] {5, 6, 7, 8, 9}, 1, 3);
NativePosixFiles.close(fd2);
assertThat(Files.readAllBytes(myfile)).isEqualTo(new byte[] {0, 1, 2, 3, 6, 7, 8});
}
}

0 comments on commit bb6e1af

Please sign in to comment.