Skip to content

Commit 22c2f9a

Browse files
tomluCopybara-Service
tomlu
authored and
Copybara-Service
committed
Automated rollback of commit 80c8ff1.
*** Reason for rollback *** Rolling forward after underlying issue has been fixed. *** Original change description *** Automated rollback of commit d50cbbe. *** 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 l... *** PiperOrigin-RevId: 181368707
1 parent 7f17d08 commit 22c2f9a

File tree

3 files changed

+85
-119
lines changed

3 files changed

+85
-119
lines changed

‎src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java

+26-40
Original file line numberDiff line numberDiff line change
@@ -257,11 +257,9 @@ protected boolean isExecutable(Path path) throws IOException {
257257
*/
258258
private void modifyPermissionBits(Path path, int permissionBits, boolean add)
259259
throws IOException {
260-
synchronized (path) {
261-
int oldMode = statInternal(path, true).getPermissions();
262-
int newMode = add ? (oldMode | permissionBits) : (oldMode & ~permissionBits);
263-
NativePosixFiles.chmod(path.toString(), newMode);
264-
}
260+
int oldMode = statInternal(path, true).getPermissions();
261+
int newMode = add ? (oldMode | permissionBits) : (oldMode & ~permissionBits);
262+
NativePosixFiles.chmod(path.toString(), newMode);
265263
}
266264

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

282280
@Override
283281
protected void chmod(Path path, int mode) throws IOException {
284-
synchronized (path) {
285-
NativePosixFiles.chmod(path.toString(), mode);
286-
}
282+
NativePosixFiles.chmod(path.toString(), mode);
287283
}
288284

289285
@Override
@@ -308,19 +304,17 @@ public boolean isFilePathCaseSensitive() {
308304

309305
@Override
310306
public boolean createDirectory(Path path) throws IOException {
311-
synchronized (path) {
312-
// Note: UNIX mkdir(2), FilesystemUtils.mkdir() and createDirectory all
313-
// have different ways of representing failure!
314-
if (NativePosixFiles.mkdir(path.toString(), 0777)) {
315-
return true; // successfully created
316-
}
307+
// Note: UNIX mkdir(2), FilesystemUtils.mkdir() and createDirectory all
308+
// have different ways of representing failure!
309+
if (NativePosixFiles.mkdir(path.toString(), 0777)) {
310+
return true; // successfully created
311+
}
317312

318-
// false => EEXIST: something is already in the way (file/dir/symlink)
319-
if (isDirectory(path, false)) {
320-
return false; // directory already existed
321-
} else {
322-
throw new IOException(path + " (File exists)");
323-
}
313+
// false => EEXIST: something is already in the way (file/dir/symlink)
314+
if (isDirectory(path, false)) {
315+
return false; // directory already existed
316+
} else {
317+
throw new IOException(path + " (File exists)");
324318
}
325319
}
326320

@@ -332,9 +326,7 @@ public void createDirectoryAndParents(Path path) throws IOException {
332326
@Override
333327
protected void createSymbolicLink(Path linkPath, PathFragment targetFragment)
334328
throws IOException {
335-
synchronized (linkPath) {
336-
NativePosixFiles.symlink(targetFragment.toString(), linkPath.toString());
337-
}
329+
NativePosixFiles.symlink(targetFragment.toString(), linkPath.toString());
338330
}
339331

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

356348
@Override
357349
public void renameTo(Path sourcePath, Path targetPath) throws IOException {
358-
synchronized (sourcePath) {
359-
NativePosixFiles.rename(sourcePath.toString(), targetPath.toString());
360-
}
350+
NativePosixFiles.rename(sourcePath.toString(), targetPath.toString());
361351
}
362352

363353
@Override
@@ -369,12 +359,10 @@ protected long getFileSize(Path path, boolean followSymlinks) throws IOException
369359
public boolean delete(Path path) throws IOException {
370360
String name = path.toString();
371361
long startTime = Profiler.nanoTimeMaybe();
372-
synchronized (path) {
373-
try {
374-
return NativePosixFiles.remove(name);
375-
} finally {
376-
profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, name);
377-
}
362+
try {
363+
return NativePosixFiles.remove(name);
364+
} finally {
365+
profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, name);
378366
}
379367
}
380368

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

386374
@Override
387375
public void setLastModifiedTime(Path path, long newTime) throws IOException {
388-
synchronized (path) {
389-
if (newTime == -1L) { // "now"
390-
NativePosixFiles.utime(path.toString(), true, 0);
391-
} else {
392-
// newTime > MAX_INT => -ve unixTime
393-
int unixTime = (int) (newTime / 1000);
394-
NativePosixFiles.utime(path.toString(), false, unixTime);
395-
}
376+
if (newTime == -1L) { // "now"
377+
NativePosixFiles.utime(path.toString(), true, 0);
378+
} else {
379+
// newTime > MAX_INT => -ve unixTime
380+
int unixTime = (int) (newTime / 1000);
381+
NativePosixFiles.utime(path.toString(), false, unixTime);
396382
}
397383
}
398384

‎src/main/java/com/google/devtools/build/lib/vfs/AbstractFileSystem.java

+9-11
Original file line numberDiff line numberDiff line change
@@ -96,18 +96,16 @@ protected OutputStream createFileOutputStream(Path path, boolean append)
9696

9797
@Override
9898
protected OutputStream getOutputStream(Path path, boolean append) throws IOException {
99-
synchronized (path) {
100-
try {
101-
return createFileOutputStream(path, append);
102-
} catch (FileNotFoundException e) {
103-
// Why does it throw a *FileNotFoundException* if it can't write?
104-
// That does not make any sense! And its in a completely different
105-
// format than in other situations, no less!
106-
if (e.getMessage().equals(path + ERR_PERMISSION_DENIED)) {
107-
throw new FileAccessException(e.getMessage());
108-
}
109-
throw e;
99+
try {
100+
return createFileOutputStream(path, append);
101+
} catch (FileNotFoundException e) {
102+
// Why does it throw a *FileNotFoundException* if it can't write?
103+
// That does not make any sense! And its in a completely different
104+
// format than in other situations, no less!
105+
if (e.getMessage().equals(path + ERR_PERMISSION_DENIED)) {
106+
throw new FileAccessException(e.getMessage());
110107
}
108+
throw e;
111109
}
112110
}
113111

‎src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java

+50-68
Original file line numberDiff line numberDiff line change
@@ -218,43 +218,29 @@ public boolean isFilePathCaseSensitive() {
218218

219219
@Override
220220
public boolean createDirectory(Path path) throws IOException {
221+
File file = getIoFile(path);
222+
if (file.mkdir()) {
223+
return true;
224+
}
221225

222-
// We always synchronize on the current path before doing it on the parent path and file system
223-
// path structure ensures that this locking order will never be reversed.
224-
// When refactoring, check that subclasses still work as expected and there can be no
225-
// deadlocks.
226-
synchronized (path) {
227-
File file = getIoFile(path);
228-
if (file.mkdir()) {
229-
return true;
230-
}
231-
232-
// We will be checking the state of the parent path as well. Synchronize on it before
233-
// attempting anything.
234-
Path parentDirectory = path.getParentDirectory();
235-
synchronized (parentDirectory) {
236-
if (fileIsSymbolicLink(file)) {
237-
throw new IOException(path + ERR_FILE_EXISTS);
238-
}
239-
if (file.isDirectory()) {
240-
return false; // directory already existed
241-
} else if (file.exists()) {
242-
throw new IOException(path + ERR_FILE_EXISTS);
243-
} else if (!file.getParentFile().exists()) {
244-
throw new FileNotFoundException(path.getParentDirectory() + ERR_NO_SUCH_FILE_OR_DIR);
245-
}
246-
// Parent directory apparently exists - try to create our directory again - protecting
247-
// against the case where parent directory would be created right before us obtaining
248-
// synchronization lock.
249-
if (file.mkdir()) {
250-
return true; // Everything is fine finally.
251-
} else if (!file.getParentFile().canWrite()) {
252-
throw new FileAccessException(path + ERR_PERMISSION_DENIED);
253-
} else {
254-
// Parent exists, is writable, yet we can't create our directory.
255-
throw new FileNotFoundException(path.getParentDirectory() + ERR_NOT_A_DIRECTORY);
256-
}
257-
}
226+
if (fileIsSymbolicLink(file)) {
227+
throw new IOException(path + ERR_FILE_EXISTS);
228+
}
229+
if (file.isDirectory()) {
230+
return false; // directory already existed
231+
} else if (file.exists()) {
232+
throw new IOException(path + ERR_FILE_EXISTS);
233+
} else if (!file.getParentFile().exists()) {
234+
throw new FileNotFoundException(path.getParentDirectory() + ERR_NO_SUCH_FILE_OR_DIR);
235+
}
236+
// Parent directory apparently exists - try to create our directory again.
237+
if (file.mkdir()) {
238+
return true; // Everything is fine finally.
239+
} else if (!file.getParentFile().canWrite()) {
240+
throw new FileAccessException(path + ERR_PERMISSION_DENIED);
241+
} else {
242+
// Parent exists, is writable, yet we can't create our directory.
243+
throw new FileNotFoundException(path.getParentDirectory() + ERR_NOT_A_DIRECTORY);
258244
}
259245
}
260246

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

324310
@Override
325311
public void renameTo(Path sourcePath, Path targetPath) throws IOException {
326-
synchronized (sourcePath) {
327-
File sourceFile = getIoFile(sourcePath);
328-
File targetFile = getIoFile(targetPath);
329-
if (!sourceFile.renameTo(targetFile)) {
330-
if (!sourceFile.exists()) {
331-
throw new FileNotFoundException(sourcePath + ERR_NO_SUCH_FILE_OR_DIR);
332-
}
333-
if (targetFile.exists()) {
334-
if (targetFile.isDirectory() && targetFile.list().length > 0) {
335-
throw new IOException(targetPath + ERR_DIRECTORY_NOT_EMPTY);
336-
} else if (sourceFile.isDirectory() && targetFile.isFile()) {
337-
throw new IOException(sourcePath + " -> " + targetPath + ERR_NOT_A_DIRECTORY);
338-
} else if (sourceFile.isFile() && targetFile.isDirectory()) {
339-
throw new IOException(sourcePath + " -> " + targetPath + ERR_IS_DIRECTORY);
340-
} else {
341-
throw new IOException(sourcePath + " -> " + targetPath + ERR_PERMISSION_DENIED);
342-
}
312+
File sourceFile = getIoFile(sourcePath);
313+
File targetFile = getIoFile(targetPath);
314+
if (!sourceFile.renameTo(targetFile)) {
315+
if (!sourceFile.exists()) {
316+
throw new FileNotFoundException(sourcePath + ERR_NO_SUCH_FILE_OR_DIR);
317+
}
318+
if (targetFile.exists()) {
319+
if (targetFile.isDirectory() && targetFile.list().length > 0) {
320+
throw new IOException(targetPath + ERR_DIRECTORY_NOT_EMPTY);
321+
} else if (sourceFile.isDirectory() && targetFile.isFile()) {
322+
throw new IOException(sourcePath + " -> " + targetPath + ERR_NOT_A_DIRECTORY);
323+
} else if (sourceFile.isFile() && targetFile.isDirectory()) {
324+
throw new IOException(sourcePath + " -> " + targetPath + ERR_IS_DIRECTORY);
343325
} else {
344-
throw new FileAccessException(sourcePath + " -> " + targetPath + ERR_PERMISSION_DENIED);
326+
throw new IOException(sourcePath + " -> " + targetPath + ERR_PERMISSION_DENIED);
345327
}
328+
} else {
329+
throw new FileAccessException(sourcePath + " -> " + targetPath + ERR_PERMISSION_DENIED);
346330
}
347331
}
348332
}
@@ -361,22 +345,20 @@ protected long getFileSize(Path path, boolean followSymlinks) throws IOException
361345
public boolean delete(Path path) throws IOException {
362346
File file = getIoFile(path);
363347
long startTime = Profiler.nanoTimeMaybe();
364-
synchronized (path) {
365-
try {
366-
if (file.delete()) {
367-
return true;
368-
}
369-
if (file.exists()) {
370-
if (file.isDirectory() && file.list().length > 0) {
371-
throw new IOException(path + ERR_DIRECTORY_NOT_EMPTY);
372-
} else {
373-
throw new IOException(path + ERR_PERMISSION_DENIED);
374-
}
348+
try {
349+
if (file.delete()) {
350+
return true;
351+
}
352+
if (file.exists()) {
353+
if (file.isDirectory() && file.list().length > 0) {
354+
throw new IOException(path + ERR_DIRECTORY_NOT_EMPTY);
355+
} else {
356+
throw new IOException(path + ERR_PERMISSION_DENIED);
375357
}
376-
return false;
377-
} finally {
378-
profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, file.getPath());
379358
}
359+
return false;
360+
} finally {
361+
profiler.logSimpleTask(startTime, ProfilerTask.VFS_DELETE, file.getPath());
380362
}
381363
}
382364

0 commit comments

Comments
 (0)