Skip to content

Commit

Permalink
Automated rollback of commit d50cbbe.
Browse files Browse the repository at this point in the history 
*** Reason for rollback ***

b/71442447

*** Original change description ***

Remove synchronization from file system.

After the path refactor we will no longer have path instances to synchronize on.

The underlying OS file systems are already naturally thread safe, that is, their internal data structures cannot be damaged. Any further synchronization (eg. races between directory creation and deletion) has to be managed at the client level.

The last attempt to do this failed because of races in FileUtils#createDirectoryAndParents on Windows. This method is now gone, rep...

***

ROLLBACK_OF=180290901
PiperOrigin-RevId: 180936132
  • Loading branch information
@tomlu
tomlu authored and Copybara-Service committed Jan 5, 2018
1 parent 3ed0159 commit 80c8ff1
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 85 deletions.
66 changes: 40 additions & 26 deletions src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,11 @@ protected boolean isExecutable(Path path) throws IOException {
*/
private void modifyPermissionBits(Path path, int permissionBits, boolean add)
throws IOException {
int oldMode = statInternal(path, true).getPermissions();
int newMode = add ? (oldMode | permissionBits) : (oldMode & ~permissionBits);
NativePosixFiles.chmod(path.toString(), newMode);
synchronized (path) {
int oldMode = statInternal(path, true).getPermissions();
int newMode = add ? (oldMode | permissionBits) : (oldMode & ~permissionBits);
NativePosixFiles.chmod(path.toString(), newMode);
}
}

@Override
Expand All @@ -279,7 +281,9 @@ protected void setExecutable(Path path, boolean executable) throws IOException {

@Override
protected void chmod(Path path, int mode) throws IOException {
NativePosixFiles.chmod(path.toString(), mode);
synchronized (path) {
NativePosixFiles.chmod(path.toString(), mode);
}
}

@Override
Expand All @@ -304,17 +308,19 @@ public boolean isFilePathCaseSensitive() {

@Override
public boolean createDirectory(Path path) throws IOException {
// Note: UNIX mkdir(2), FilesystemUtils.mkdir() and createDirectory all
// have different ways of representing failure!
if (NativePosixFiles.mkdir(path.toString(), 0777)) {
return true; // successfully created
}
synchronized (path) {
// Note: UNIX mkdir(2), FilesystemUtils.mkdir() and createDirectory all
// have different ways of representing failure!
if (NativePosixFiles.mkdir(path.toString(), 0777)) {
return true; // successfully created
}

// false => EEXIST: something is already in the way (file/dir/symlink)
if (isDirectory(path, false)) {
return false; // directory already existed
} else {
throw new IOException(path + " (File exists)");
// false => EEXIST: something is already in the way (file/dir/symlink)
if (isDirectory(path, false)) {
return false; // directory already existed
} else {
throw new IOException(path + " (File exists)");
}
}
}

Expand All @@ -326,7 +332,9 @@ public void createDirectoryAndParents(Path path) throws IOException {
@Override
protected void createSymbolicLink(Path linkPath, PathFragment targetFragment)
throws IOException {
NativePosixFiles.symlink(targetFragment.toString(), linkPath.toString());
synchronized (linkPath) {
NativePosixFiles.symlink(targetFragment.toString(), linkPath.toString());
}
}

@Override
Expand All @@ -347,7 +355,9 @@ protected PathFragment readSymbolicLink(Path path) throws IOException {

@Override
public void renameTo(Path sourcePath, Path targetPath) throws IOException {
NativePosixFiles.rename(sourcePath.toString(), targetPath.toString());
synchronized (sourcePath) {
NativePosixFiles.rename(sourcePath.toString(), targetPath.toString());
}
}

@Override
Expand All @@ -359,10 +369,12 @@ protected long getFileSize(Path path, boolean followSymlinks) throws IOException
public boolean delete(Path path) throws IOException {
String name = path.toString();
long startTime = Profiler.nanoTimeMaybe();
try {
return NativePosixFiles.remove(name);
} finally {
profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, name);
synchronized (path) {
try {
return NativePosixFiles.remove(name);
} finally {
profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, name);
}
}
}

Expand All @@ -373,12 +385,14 @@ protected long getLastModifiedTime(Path path, boolean followSymlinks) throws IOE

@Override
public void setLastModifiedTime(Path path, long newTime) throws IOException {
if (newTime == -1L) { // "now"
NativePosixFiles.utime(path.toString(), true, 0);
} else {
// newTime > MAX_INT => -ve unixTime
int unixTime = (int) (newTime / 1000);
NativePosixFiles.utime(path.toString(), false, unixTime);
synchronized (path) {
if (newTime == -1L) { // "now"
NativePosixFiles.utime(path.toString(), true, 0);
} else {
// newTime > MAX_INT => -ve unixTime
int unixTime = (int) (newTime / 1000);
NativePosixFiles.utime(path.toString(), false, unixTime);
}
}
}

Expand Down
20 changes: 11 additions & 9 deletions src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,18 @@ protected OutputStream createFileOutputStream(Path path, boolean append)

@Override
protected OutputStream getOutputStream(Path path, boolean append) throws IOException {
try {
return createFileOutputStream(path, append);
} catch (FileNotFoundException e) {
// Why does it throw a *FileNotFoundException* if it can't write?
// That does not make any sense! And its in a completely different
// format than in other situations, no less!
if (e.getMessage().equals(path + ERR_PERMISSION_DENIED)) {
throw new FileAccessException(e.getMessage());
synchronized (path) {
try {
return createFileOutputStream(path, append);
} catch (FileNotFoundException e) {
// Why does it throw a *FileNotFoundException* if it can't write?
// That does not make any sense! And its in a completely different
// format than in other situations, no less!
if (e.getMessage().equals(path + ERR_PERMISSION_DENIED)) {
throw new FileAccessException(e.getMessage());
}
throw e;
}
throw e;
}
}

Expand Down
118 changes: 68 additions & 50 deletions src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,29 +218,43 @@ public boolean isFilePathCaseSensitive() {

@Override
public boolean createDirectory(Path path) throws IOException {
File file = getIoFile(path);
if (file.mkdir()) {
return true;
}

if (fileIsSymbolicLink(file)) {
throw new IOException(path + ERR_FILE_EXISTS);
}
if (file.isDirectory()) {
return false; // directory already existed
} else if (file.exists()) {
throw new IOException(path + ERR_FILE_EXISTS);
} else if (!file.getParentFile().exists()) {
throw new FileNotFoundException(path.getParentDirectory() + ERR_NO_SUCH_FILE_OR_DIR);
}
// Parent directory apparently exists - try to create our directory again.
if (file.mkdir()) {
return true; // Everything is fine finally.
} else if (!file.getParentFile().canWrite()) {
throw new FileAccessException(path + ERR_PERMISSION_DENIED);
} else {
// Parent exists, is writable, yet we can't create our directory.
throw new FileNotFoundException(path.getParentDirectory() + ERR_NOT_A_DIRECTORY);
// We always synchronize on the current path before doing it on the parent path and file system
// path structure ensures that this locking order will never be reversed.
// When refactoring, check that subclasses still work as expected and there can be no
// deadlocks.
synchronized (path) {
File file = getIoFile(path);
if (file.mkdir()) {
return true;
}

// We will be checking the state of the parent path as well. Synchronize on it before
// attempting anything.
Path parentDirectory = path.getParentDirectory();
synchronized (parentDirectory) {
if (fileIsSymbolicLink(file)) {
throw new IOException(path + ERR_FILE_EXISTS);
}
if (file.isDirectory()) {
return false; // directory already existed
} else if (file.exists()) {
throw new IOException(path + ERR_FILE_EXISTS);
} else if (!file.getParentFile().exists()) {
throw new FileNotFoundException(path.getParentDirectory() + ERR_NO_SUCH_FILE_OR_DIR);
}
// Parent directory apparently exists - try to create our directory again - protecting
// against the case where parent directory would be created right before us obtaining
// synchronization lock.
if (file.mkdir()) {
return true; // Everything is fine finally.
} else if (!file.getParentFile().canWrite()) {
throw new FileAccessException(path + ERR_PERMISSION_DENIED);
} else {
// Parent exists, is writable, yet we can't create our directory.
throw new FileNotFoundException(path.getParentDirectory() + ERR_NOT_A_DIRECTORY);
}
}
}
}

Expand Down Expand Up @@ -309,24 +323,26 @@ protected PathFragment readSymbolicLink(Path path) throws IOException {

@Override
public void renameTo(Path sourcePath, Path targetPath) throws IOException {
File sourceFile = getIoFile(sourcePath);
File targetFile = getIoFile(targetPath);
if (!sourceFile.renameTo(targetFile)) {
if (!sourceFile.exists()) {
throw new FileNotFoundException(sourcePath + ERR_NO_SUCH_FILE_OR_DIR);
}
if (targetFile.exists()) {
if (targetFile.isDirectory() && targetFile.list().length > 0) {
throw new IOException(targetPath + ERR_DIRECTORY_NOT_EMPTY);
} else if (sourceFile.isDirectory() && targetFile.isFile()) {
throw new IOException(sourcePath + " -> " + targetPath + ERR_NOT_A_DIRECTORY);
} else if (sourceFile.isFile() && targetFile.isDirectory()) {
throw new IOException(sourcePath + " -> " + targetPath + ERR_IS_DIRECTORY);
synchronized (sourcePath) {
File sourceFile = getIoFile(sourcePath);
File targetFile = getIoFile(targetPath);
if (!sourceFile.renameTo(targetFile)) {
if (!sourceFile.exists()) {
throw new FileNotFoundException(sourcePath + ERR_NO_SUCH_FILE_OR_DIR);
}
if (targetFile.exists()) {
if (targetFile.isDirectory() && targetFile.list().length > 0) {
throw new IOException(targetPath + ERR_DIRECTORY_NOT_EMPTY);
} else if (sourceFile.isDirectory() && targetFile.isFile()) {
throw new IOException(sourcePath + " -> " + targetPath + ERR_NOT_A_DIRECTORY);
} else if (sourceFile.isFile() && targetFile.isDirectory()) {
throw new IOException(sourcePath + " -> " + targetPath + ERR_IS_DIRECTORY);
} else {
throw new IOException(sourcePath + " -> " + targetPath + ERR_PERMISSION_DENIED);
}
} else {
throw new IOException(sourcePath + " -> " + targetPath + ERR_PERMISSION_DENIED);
throw new FileAccessException(sourcePath + " -> " + targetPath + ERR_PERMISSION_DENIED);
}
} else {
throw new FileAccessException(sourcePath + " -> " + targetPath + ERR_PERMISSION_DENIED);
}
}
}
Expand All @@ -345,20 +361,22 @@ protected long getFileSize(Path path, boolean followSymlinks) throws IOException
public boolean delete(Path path) throws IOException {
File file = getIoFile(path);
long startTime = Profiler.nanoTimeMaybe();
try {
if (file.delete()) {
return true;
}
if (file.exists()) {
if (file.isDirectory() && file.list().length > 0) {
throw new IOException(path + ERR_DIRECTORY_NOT_EMPTY);
} else {
throw new IOException(path + ERR_PERMISSION_DENIED);
synchronized (path) {
try {
if (file.delete()) {
return true;
}
if (file.exists()) {
if (file.isDirectory() && file.list().length > 0) {
throw new IOException(path + ERR_DIRECTORY_NOT_EMPTY);
} else {
throw new IOException(path + ERR_PERMISSION_DENIED);
}
}
return false;
} finally {
profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, file.getPath());
}
return false;
} finally {
profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, file.getPath());
}
}

Expand Down

0 comments on commit 80c8ff1

Please sign in to comment.