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, replaced by a method from the Java framework that knows how to synchronize.

PiperOrigin-RevId: 180290901

3 files changed, 85 insertions, 119 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java
index 0af17cfb12..033a6a692d 100644
--- a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java
@@ -257,11 +257,9 @@ public class UnixFileSystem extends AbstractFileSystemWithCustomStat {
    */
   private void modifyPermissionBits(Path path, int permissionBits, boolean add)
     throws IOException {
-    synchronized (path) {
-      int oldMode = statInternal(path, true).getPermissions();
-      int newMode = add ? (oldMode | permissionBits) : (oldMode & ~permissionBits);
-      NativePosixFiles.chmod(path.toString(), newMode);
-    }
+    int oldMode = statInternal(path, true).getPermissions();
+    int newMode = add ? (oldMode | permissionBits) : (oldMode & ~permissionBits);
+    NativePosixFiles.chmod(path.toString(), newMode);
   }
 
   @Override
@@ -281,9 +279,7 @@ public class UnixFileSystem extends AbstractFileSystemWithCustomStat {
 
   @Override
   protected void chmod(Path path, int mode) throws IOException {
-    synchronized (path) {
-      NativePosixFiles.chmod(path.toString(), mode);
-    }
+    NativePosixFiles.chmod(path.toString(), mode);
   }
 
   @Override
@@ -308,19 +304,17 @@ public class UnixFileSystem extends AbstractFileSystemWithCustomStat {
 
   @Override
   public boolean createDirectory(Path path) throws IOException {
-    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
-      }
+    // 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)");
     }
   }
 
@@ -332,9 +326,7 @@ public class UnixFileSystem extends AbstractFileSystemWithCustomStat {
   @Override
   protected void createSymbolicLink(Path linkPath, PathFragment targetFragment)
       throws IOException {
-    synchronized (linkPath) {
-      NativePosixFiles.symlink(targetFragment.toString(), linkPath.toString());
-    }
+    NativePosixFiles.symlink(targetFragment.toString(), linkPath.toString());
   }
 
   @Override
@@ -355,9 +347,7 @@ public class UnixFileSystem extends AbstractFileSystemWithCustomStat {
 
   @Override
   public void renameTo(Path sourcePath, Path targetPath) throws IOException {
-    synchronized (sourcePath) {
-      NativePosixFiles.rename(sourcePath.toString(), targetPath.toString());
-    }
+    NativePosixFiles.rename(sourcePath.toString(), targetPath.toString());
   }
 
   @Override
@@ -369,12 +359,10 @@ public class UnixFileSystem extends AbstractFileSystemWithCustomStat {
   public boolean delete(Path path) throws IOException {
     String name = path.toString();
     long startTime = Profiler.nanoTimeMaybe();
-    synchronized (path) {
-      try {
-        return NativePosixFiles.remove(name);
-      } finally {
-        profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, name);
-      }
+    try {
+      return NativePosixFiles.remove(name);
+    } finally {
+      profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, name);
     }
   }
 
@@ -385,14 +373,12 @@ public class UnixFileSystem extends AbstractFileSystemWithCustomStat {
 
   @Override
   public void setLastModifiedTime(Path path, long newTime) throws IOException {
-    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);
-      }
+    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);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java
index 743bf37dd0..4d47205843 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java
@@ -96,18 +96,16 @@ abstract class AbstractFileSystem extends FileSystem {
 
   @Override
   protected OutputStream getOutputStream(Path path, boolean append) throws IOException {
-    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;
+    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;
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
index 0dac1c4725..5bba53533f 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java
@@ -218,43 +218,29 @@ public class JavaIoFileSystem extends AbstractFileSystemWithCustomStat {
 
   @Override
   public boolean createDirectory(Path path) throws IOException {
+    File file = getIoFile(path);
+    if (file.mkdir()) {
+      return true;
+    }
 
-    // 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);
-        }
-      }
+    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);
     }
   }
 
@@ -323,26 +309,24 @@ public class JavaIoFileSystem extends AbstractFileSystemWithCustomStat {
 
   @Override
   public void renameTo(Path sourcePath, Path targetPath) throws IOException {
-    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);
-          }
+    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 FileAccessException(sourcePath + " -> " + targetPath + ERR_PERMISSION_DENIED);
+          throw new IOException(sourcePath + " -> " + targetPath  + ERR_PERMISSION_DENIED);
         }
+      } else {
+        throw new FileAccessException(sourcePath + " -> " + targetPath + ERR_PERMISSION_DENIED);
       }
     }
   }
@@ -361,22 +345,20 @@ public class JavaIoFileSystem extends AbstractFileSystemWithCustomStat {
   public boolean delete(Path path) throws IOException {
     File file = getIoFile(path);
     long startTime = Profiler.nanoTimeMaybe();
-    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);
-          }
+    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());
     }
   }