-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve FileUtils.deleteRecursively diagnostics. #5430
Conversation
@@ -59,19 +61,23 @@ public static void cleanDirectory(File path) { | |||
} | |||
|
|||
public static void deleteRecursively(File file) { | |||
if (!file.exists()) { | |||
if (!Files.exists(file.toPath(), LinkOption.NOFOLLOW_LINKS)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any reason to expect that the methods you're substituting don't perform identically to the originals?
Are you certain we shouldn't follow links?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The NOFOLLOW_LINKS is important, otherwise a symlink to a non-existing directory will cause the file to not exist, and thus it is not removed - leaving crap around.
The File.delete() member method does not throw exceptions, it just returns false (which the original code asserted on); meaning you do not have access to the underlying IOException that caused problems. The new method throws an IOException which we can turn into an UncheckedIOException with the filename, so at least you know why it failed (permission denied, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this "exists" check makes sense for the top-level caller to match the semantics today, but we might want to restructure the implementation so that the recursive calls don't need to make that check. We might think it's a "good safety check", but there isn't atomicity guarantees with it; exists can succeed now, and fail later.
It's worth noting, the semantics today and with this PR afaict is to recurse into symlinked directories and delete all of those files - is that something we want / need? We probably shouldn't change this methods behavior in that regard, but we should probably document it and provide an alternative that we may think of as safer.
return; | ||
} | ||
if (file.isDirectory()) { | ||
File f[] = file.listFiles(); | ||
final File[] files = file.listFiles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we might prefer
Files.walkFileTree(file.toPath(), new SimpleFileVisitor<>() {
@Override
public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
if (exc != null) {
throw exc;
}
Files.deleteIfExists(dir);
return FileVisitResult.CONTINUE;
}
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
Files.deleteIfExists(file);
return FileVisitResult.CONTINUE;
}
});
it looks like walkFileTree does not follow links by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That isn't clearly better and would require testing it vs. the ported code which has already been tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing that makes it better if it's not recursive.
Fixes #5431