From d8451f1d298416690b74d8b271934eb1642ea5d5 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 23 Sep 2021 09:51:06 +0200 Subject: [PATCH 1/6] FileSystem.Unix: Directory.Delete: remove per item syscall. By recursing using FileSystemEnumerable we know the file type and can omit the stat calls made by DirectoryInfo.Exists. For the top level path, we can omit the call also and handle non-directories when rmdir errno is ENOTDIR. For the recursive case we can avoid recursion when the top level path rmdir succeeds immediately. FileSystemEntry is updated so IsSymbolicLink remembers the file is symbolic link and does not make a syscall for it. --- .../tests/Directory/Delete.cs | 29 +++++ .../src/System/IO/FileSystem.Unix.cs | 111 ++++++++++-------- 2 files changed, 93 insertions(+), 47 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs index b264ec4c3b60e..f6f71a780cf3e 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs @@ -119,6 +119,35 @@ public void DeletingSymLinkDoesntDeleteTarget() Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); } + [ConditionalFact(nameof(CanCreateSymbolicLinks))] + public void RecursiveDeletingDoesntFollowLinks() + { + var target = GetTestFilePath(); + Directory.CreateDirectory(target); + + var fileInTarget = Path.Combine(target, GetTestFileName()); + File.WriteAllText(fileInTarget, ""); + + var linkParent = GetTestFilePath(); + Directory.CreateDirectory(linkParent); + + var linkPath = Path.Combine(linkParent, GetTestFileName()); + Assert.True(MountHelper.CreateSymbolicLink(linkPath, target, isDirectory: true)); + + // Both the symlink and the target exist + Assert.True(Directory.Exists(target), "target should exist"); + Assert.True(Directory.Exists(linkPath), "linkPath should exist"); + Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); + + // Delete the parent folder of the symlink. + Directory.Delete(linkParent, true); + + // Target should still exist + Assert.True(Directory.Exists(target), "target should still exist"); + Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); + Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); + } + [ConditionalFact(nameof(UsingNewNormalization))] public void ExtendedDirectoryWithSubdirectories() { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index e23ae9e9e28a8..4d29f3bd63f90 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; +using System.IO.Enumeration; using System.Text; using Microsoft.Win32.SafeHandles; @@ -433,69 +434,64 @@ public static void MoveDirectory(string sourceFullPath, string destFullPath) public static void RemoveDirectory(string fullPath, bool recursive) { - var di = new DirectoryInfo(fullPath); - if (!di.Exists) + if (!TryRemoveDirectory(fullPath, topLevel: true, throwWhenNotEmpty: !recursive)) { - throw Interop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(), fullPath, isDirectory: true); + RemoveDirectoryRecursive(fullPath); } - RemoveDirectoryInternal(di, recursive, throwOnTopLevelDirectoryNotFound: true); } - private static void RemoveDirectoryInternal(DirectoryInfo directory, bool recursive, bool throwOnTopLevelDirectoryNotFound) + private static void RemoveDirectoryRecursive(string fullPath) { Exception? firstException = null; - if ((directory.Attributes & FileAttributes.ReparsePoint) != 0) + try { - DeleteFile(directory.FullName); - return; - } + foreach ((string childPath, bool isDirectory) in + new FileSystemEnumerable<(string, bool)>( + fullPath, + static (ref FileSystemEntry entry) => + { + // Don't report symlinks to directories as directories. + bool isRealDirectory = !entry.IsSymbolicLink && entry.IsDirectory; - if (recursive) - { - try + return (entry.ToFullPath(), isRealDirectory); + }, + EnumerationOptions.Compatible)) { - foreach (string item in Directory.EnumerateFileSystemEntries(directory.FullName)) + try { - if (!ShouldIgnoreDirectory(Path.GetFileName(item))) + if (isDirectory) { - try - { - var childDirectory = new DirectoryInfo(item); - if (childDirectory.Exists) - { - RemoveDirectoryInternal(childDirectory, recursive, throwOnTopLevelDirectoryNotFound: false); - } - else - { - DeleteFile(item); - } - } - catch (Exception exc) - { - if (firstException != null) - { - firstException = exc; - } - } + RemoveDirectoryRecursive(childPath); + } + else + { + DeleteFile(childPath); } } - } - catch (Exception exc) - { - if (firstException != null) + catch (Exception ex) { - firstException = exc; + firstException ??= ex; } } + } + catch (Exception exc) + { + firstException ??= exc; + } - if (firstException != null) - { - throw firstException; - } + if (firstException != null) + { + throw firstException; } - if (Interop.Sys.RmDir(directory.FullName) < 0) + bool removed = TryRemoveDirectory(fullPath); + Debug.Assert(removed); + } + + private static bool TryRemoveDirectory(string fullPath, bool topLevel = false, bool throwWhenNotEmpty = true) + { + if (Interop.Sys.RmDir(fullPath) < 0) { Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo(); switch (errorInfo.Error) @@ -504,17 +500,38 @@ private static void RemoveDirectoryInternal(DirectoryInfo directory, bool recurs case Interop.Error.EPERM: case Interop.Error.EROFS: case Interop.Error.EISDIR: - throw new IOException(SR.Format(SR.UnauthorizedAccess_IODenied_Path, directory.FullName)); // match Win32 exception + throw new IOException(SR.Format(SR.UnauthorizedAccess_IODenied_Path, fullPath)); // match Win32 exception case Interop.Error.ENOENT: - if (!throwOnTopLevelDirectoryNotFound) + if (!topLevel) { - return; + return true; + } + goto default; + case Interop.Error.ENOTDIR: + if (topLevel) + { + if (!DirectoryExists(fullPath)) + { + throw Interop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(), fullPath, isDirectory: true); + } + + // Path is symbolic link to a directory. + DeleteFile(fullPath); + return true; + } + goto default; + case Interop.Error.ENOTEMPTY: + if (!throwWhenNotEmpty) + { + return false; } goto default; default: - throw Interop.GetExceptionForIoErrno(errorInfo, directory.FullName, isDirectory: true); + throw Interop.GetExceptionForIoErrno(errorInfo, fullPath, isDirectory: true); } } + + return true; } /// Determines whether the specified directory name should be ignored. From ead0f4772e1a917ad853920239642be784350892 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 18 Nov 2021 19:35:24 +0100 Subject: [PATCH 2/6] PR feedback --- .../src/System/IO/FileSystem.Unix.cs | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs index 4d29f3bd63f90..05b782147a8e3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs @@ -434,8 +434,12 @@ public static void MoveDirectory(string sourceFullPath, string destFullPath) public static void RemoveDirectory(string fullPath, bool recursive) { - if (!TryRemoveDirectory(fullPath, topLevel: true, throwWhenNotEmpty: !recursive)) + // Delete the directory. + // If we're recursing, don't throw when it is not empty, and perform a recursive remove. + if (!RemoveEmptyDirectory(fullPath, topLevel: true, throwWhenNotEmpty: !recursive)) { + Debug.Assert(recursive); + RemoveDirectoryRecursive(fullPath); } } @@ -446,17 +450,16 @@ private static void RemoveDirectoryRecursive(string fullPath) try { - foreach ((string childPath, bool isDirectory) in - new FileSystemEnumerable<(string, bool)>( - fullPath, - static (ref FileSystemEntry entry) => - { - // Don't report symlinks to directories as directories. - bool isRealDirectory = !entry.IsSymbolicLink && entry.IsDirectory; - - return (entry.ToFullPath(), isRealDirectory); - }, - EnumerationOptions.Compatible)) + var fse = new FileSystemEnumerable<(string, bool)>(fullPath, + static (ref FileSystemEntry entry) => + { + // Don't report symlinks to directories as directories. + bool isRealDirectory = !entry.IsSymbolicLink && entry.IsDirectory; + return (entry.ToFullPath(), isRealDirectory); + }, + EnumerationOptions.Compatible); + + foreach ((string childPath, bool isDirectory) in fse) { try { @@ -485,11 +488,10 @@ private static void RemoveDirectoryRecursive(string fullPath) throw firstException; } - bool removed = TryRemoveDirectory(fullPath); - Debug.Assert(removed); + RemoveEmptyDirectory(fullPath); } - private static bool TryRemoveDirectory(string fullPath, bool topLevel = false, bool throwWhenNotEmpty = true) + private static bool RemoveEmptyDirectory(string fullPath, bool topLevel = false, bool throwWhenNotEmpty = true) { if (Interop.Sys.RmDir(fullPath) < 0) { @@ -502,12 +504,15 @@ private static bool TryRemoveDirectory(string fullPath, bool topLevel = false, b case Interop.Error.EISDIR: throw new IOException(SR.Format(SR.UnauthorizedAccess_IODenied_Path, fullPath)); // match Win32 exception case Interop.Error.ENOENT: + // When we're recursing, don't throw for items that go missing. if (!topLevel) { return true; } goto default; case Interop.Error.ENOTDIR: + // When the top-level path is a symlink to a directory, delete the link. + // In other cases, throw because we expect path to be a real directory. if (topLevel) { if (!DirectoryExists(fullPath)) @@ -515,7 +520,6 @@ private static bool TryRemoveDirectory(string fullPath, bool topLevel = false, b throw Interop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(), fullPath, isDirectory: true); } - // Path is symbolic link to a directory. DeleteFile(fullPath); return true; } From 599514a521b1e10433a73b8e442de75aeeff95ba Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 18 Nov 2021 19:46:47 +0100 Subject: [PATCH 3/6] test: use Directory.CreateSymbolicLink --- src/libraries/System.IO.FileSystem/tests/File/Delete.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.IO.FileSystem/tests/File/Delete.cs b/src/libraries/System.IO.FileSystem/tests/File/Delete.cs index b696838619b0d..2e0f828110a4f 100644 --- a/src/libraries/System.IO.FileSystem/tests/File/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/File/Delete.cs @@ -88,7 +88,7 @@ public void DeletingSymLinkDoesntDeleteTarget() var linkPath = GetTestFilePath(); File.Create(path).Dispose(); - Assert.True(MountHelper.CreateSymbolicLink(linkPath, path, isDirectory: false)); + Assert.NotNull(Directory.CreateSymbolicLink(linkPath, path)); // Both the symlink and the target exist Assert.True(File.Exists(path), "path should exist"); From 63953d709e928e499484d7678d3f8f65226152ef Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 22 Nov 2021 15:24:25 +0100 Subject: [PATCH 4/6] Run test against different Delete APIs --- .../tests/Directory/Delete.cs | 58 +++++++++---------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs index f6f71a780cf3e..1e12662b971ca 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs @@ -119,35 +119,6 @@ public void DeletingSymLinkDoesntDeleteTarget() Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); } - [ConditionalFact(nameof(CanCreateSymbolicLinks))] - public void RecursiveDeletingDoesntFollowLinks() - { - var target = GetTestFilePath(); - Directory.CreateDirectory(target); - - var fileInTarget = Path.Combine(target, GetTestFileName()); - File.WriteAllText(fileInTarget, ""); - - var linkParent = GetTestFilePath(); - Directory.CreateDirectory(linkParent); - - var linkPath = Path.Combine(linkParent, GetTestFileName()); - Assert.True(MountHelper.CreateSymbolicLink(linkPath, target, isDirectory: true)); - - // Both the symlink and the target exist - Assert.True(Directory.Exists(target), "target should exist"); - Assert.True(Directory.Exists(linkPath), "linkPath should exist"); - Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); - - // Delete the parent folder of the symlink. - Directory.Delete(linkParent, true); - - // Target should still exist - Assert.True(Directory.Exists(target), "target should still exist"); - Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); - Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); - } - [ConditionalFact(nameof(UsingNewNormalization))] public void ExtendedDirectoryWithSubdirectories() { @@ -319,5 +290,34 @@ public void RecursiveDelete_ShouldThrowIOExceptionIfContainedFileInUse() } Assert.True(testDir.Exists); } + + [ConditionalFact(nameof(CanCreateSymbolicLinks))] + public void RecursiveDeletingDoesntFollowLinks() + { + var target = GetTestFilePath(); + Directory.CreateDirectory(target); + + var fileInTarget = Path.Combine(target, GetTestFileName()); + File.WriteAllText(fileInTarget, ""); + + var linkParent = GetTestFilePath(); + Directory.CreateDirectory(linkParent); + + var linkPath = Path.Combine(linkParent, GetTestFileName()); + Assert.NotNull(Directory.CreateSymbolicLink(linkPath, target)); + + // Both the symlink and the target exist + Assert.True(Directory.Exists(target), "target should exist"); + Assert.True(Directory.Exists(linkPath), "linkPath should exist"); + Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); + + // Delete the parent folder of the symlink. + Delete(linkParent, true); + + // Target should still exist + Assert.True(Directory.Exists(target), "target should still exist"); + Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); + Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); + } } } From edb54b201d4e3ffc3f10a1011e31b50f494b6034 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 23 Nov 2021 11:11:32 +0100 Subject: [PATCH 5/6] test: update Exists check for Windows --- src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs index 1e12662b971ca..30872f3972775 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs @@ -308,7 +308,7 @@ public void RecursiveDeletingDoesntFollowLinks() // Both the symlink and the target exist Assert.True(Directory.Exists(target), "target should exist"); - Assert.True(Directory.Exists(linkPath), "linkPath should exist"); + Assert.True(Directory.Exists(linkPath) || File.Exists(linkPath), "linkPath should exist"); Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); // Delete the parent folder of the symlink. @@ -316,7 +316,7 @@ public void RecursiveDeletingDoesntFollowLinks() // Target should still exist Assert.True(Directory.Exists(target), "target should still exist"); - Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); + Assert.False(Directory.Exists(linkPath) || File.Exists(linkPath), "linkPath should no longer exist"); Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); } } From 90147934804b022f4be0eb608a67f7f0546a3f58 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 23 Nov 2021 13:43:22 +0100 Subject: [PATCH 6/6] Fix tests --- src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs | 4 ++-- src/libraries/System.IO.FileSystem/tests/File/Delete.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs index 30872f3972775..1e12662b971ca 100644 --- a/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs @@ -308,7 +308,7 @@ public void RecursiveDeletingDoesntFollowLinks() // Both the symlink and the target exist Assert.True(Directory.Exists(target), "target should exist"); - Assert.True(Directory.Exists(linkPath) || File.Exists(linkPath), "linkPath should exist"); + Assert.True(Directory.Exists(linkPath), "linkPath should exist"); Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); // Delete the parent folder of the symlink. @@ -316,7 +316,7 @@ public void RecursiveDeletingDoesntFollowLinks() // Target should still exist Assert.True(Directory.Exists(target), "target should still exist"); - Assert.False(Directory.Exists(linkPath) || File.Exists(linkPath), "linkPath should no longer exist"); + Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist"); Assert.True(File.Exists(fileInTarget), "fileInTarget should exist"); } } diff --git a/src/libraries/System.IO.FileSystem/tests/File/Delete.cs b/src/libraries/System.IO.FileSystem/tests/File/Delete.cs index 2e0f828110a4f..b696838619b0d 100644 --- a/src/libraries/System.IO.FileSystem/tests/File/Delete.cs +++ b/src/libraries/System.IO.FileSystem/tests/File/Delete.cs @@ -88,7 +88,7 @@ public void DeletingSymLinkDoesntDeleteTarget() var linkPath = GetTestFilePath(); File.Create(path).Dispose(); - Assert.NotNull(Directory.CreateSymbolicLink(linkPath, path)); + Assert.True(MountHelper.CreateSymbolicLink(linkPath, path, isDirectory: false)); // Both the symlink and the target exist Assert.True(File.Exists(path), "path should exist");