Skip to content

Commit

Permalink
Take windows junctions into account with --watchfs
Browse files Browse the repository at this point in the history
Related PR: #11334 I enabled --watchfs for windows under --experimental_windows_watchfs.

Related issues: #11619:
1. (infinite) recursion occurs on the convenience symlinks when they are created while watch is active. This also causes a delay on the first bazel invocation after the symlinks are created.
2. quite often an overflow warning occurs `WARNING: Overflow when watching local filesystem for changes...`. I think this is because the watcher also watches the bazel-* symlinks with many file changes due to being output directories.

The reason that this behavior is different on windows seems to be that the convenience symlinks are actually created as junctions. I have tried changed this logic to creating actually directory symlinks which seemed to resolve the issues. This however could be a breaking change on some systems because creating symlinks requires more privileges than creating junctions.

~~Therefore I propose the following changes:~~
~~1. Watch the root directory non-recursively. As it's done on non windows systems.~~
~~2. Watch each direct sub-directory recursively. Excluding junctions.~~

~~The downside of this approach is that the convenience symlinks must be at the root of the workspace. When using --symlink_prefix with a sub-directory (which is the case for in the linked issue) both issues would still occur.~~

I have updated the PR to reflect my comment below because this also resolves the above mentioned downside and is a simpler solution. I have tested several scenarios with regards to the folder locking but was able to delete workspace folders from explorer, ide and cli every time.

Closes #12054.

PiperOrigin-RevId: 330496447
  • Loading branch information
tomdegoede authored and copybara-github committed Sep 8, 2020
1 parent 39123f3 commit 0cdd71f
Showing 1 changed file with 24 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.common.options.OptionsProvider;
import com.sun.nio.file.ExtendedWatchEventModifier;
import java.io.IOException;
import java.nio.file.ClosedWatchServiceException;
import java.nio.file.FileSystems;
Expand Down Expand Up @@ -120,24 +119,7 @@ public View getCurrentView(OptionsProvider options) throws BrokenDiffAwarenessEx
Set<Path> modifiedAbsolutePaths;
if (isFirstCall()) {
try {
// Due to a known issue nested watches may result in errors on windows:
// https://bugs.openjdk.java.net/browse/JDK-6972833
// Therefore on windows we register using the special ExtendedWatchEventModifier.FILE_TREE
// This watches a folder recursively so there is no need to apply this ourselves
if (isWindows) {
WatchKey key =
watchRootPath.register(
watchService,
new Kind<?>[] {
StandardWatchEventKinds.ENTRY_CREATE,
StandardWatchEventKinds.ENTRY_MODIFY,
StandardWatchEventKinds.ENTRY_DELETE,
},
ExtendedWatchEventModifier.FILE_TREE);
watchKeyToDirBiMap.put(key, watchRootPath);
} else {
registerSubDirectoriesAndReturnContents(watchRootPath);
}
registerSubDirectories(watchRootPath);
} catch (IOException e) {
close();
throw new BrokenDiffAwarenessException(
Expand Down Expand Up @@ -273,6 +255,12 @@ private Set<Path> collectChanges() throws BrokenDiffAwarenessException, IOExcept
return changedPaths;
}

/** Traverses directory tree to register subdirectories. */
private void registerSubDirectories(Path rootDir) throws IOException {
// Note that this does not follow symlinks.
Files.walkFileTree(rootDir, new WatcherFileVisitor());
}

/**
* Traverses directory tree to register subdirectories. Returns all paths traversed (as absolute
* paths).
Expand All @@ -293,6 +281,10 @@ private WatcherFileVisitor(Set<Path> visitedPaths) {
this.visitedAbsolutePaths = visitedPaths;
}

private WatcherFileVisitor() {
this.visitedAbsolutePaths = new HashSet<>();
}

@Override
public FileVisitResult visitFile(Path path, BasicFileAttributes attrs) {
Preconditions.checkState(path.isAbsolute(), path);
Expand All @@ -303,22 +295,24 @@ public FileVisitResult visitFile(Path path, BasicFileAttributes attrs) {
@Override
public FileVisitResult preVisitDirectory(Path path, BasicFileAttributes attrs)
throws IOException {
// Do not traverse the bazel-* convenience symlinks. On windows these are created as
// junctions.
if (isWindows && attrs.isOther()) {
return FileVisitResult.SKIP_SUBTREE;
}

// It's important that we register the directory before we visit its children. This way we
// are guaranteed to see new files/directories either on this #getDiff or the next one.
// Otherwise, e.g., an intra-build creation of a child directory will be forever missed if it
// happens before the directory is listed as part of the visitation.
Preconditions.checkState(path.isAbsolute(), path);
// On windows we register the root path with ExtendedWatchEventModifier.FILE_TREE
// Therefore there is no need to register recursive watchers
if (!isWindows) {
WatchKey key =
path.register(
watchService,
StandardWatchEventKinds.ENTRY_CREATE,
StandardWatchEventKinds.ENTRY_MODIFY,
StandardWatchEventKinds.ENTRY_DELETE);
watchKeyToDirBiMap.put(key, path);
}
WatchKey key =
path.register(
watchService,
StandardWatchEventKinds.ENTRY_CREATE,
StandardWatchEventKinds.ENTRY_MODIFY,
StandardWatchEventKinds.ENTRY_DELETE);
watchKeyToDirBiMap.put(key, path);
visitedAbsolutePaths.add(path);
return FileVisitResult.CONTINUE;
}
Expand Down

2 comments on commit 0cdd71f

@konste
Copy link

@konste konste commented on 0cdd71f Nov 26, 2020

Choose a reason for hiding this comment

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

@tomdegoede As far as I understand --watchfs feature only helps when the sources are in the root workspace directly. In our case the root workspace is just an orchestrator for the build and does not carry any actual source code. All the source code comes from http_archive and git_repository rules. Does it mean --watchfs is practically useless in our scenario?

@tomdegoede
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes unfortunately that's correct. The external folder is still manually checked (and the bazel-out as well) which is hurting the performance for us as well. Hopefully someone can rewrite the diffing for external dependencies to use watchfs in the future as well. You can look at src\main\java\com\google\devtools\build\lib\skyframe\SequencedSkyframeExecutor.java handleDiffs for more details.

Please sign in to comment.