Skip to content
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.

Sign up for GitHub

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

Jump to bottom

FileSystem.Unix: Directory.Delete: remove per item syscall. #59520

Merged
merged 6 commits into from
Nov 23, 2021
Merged

FileSystem.Unix: Directory.Delete: remove per item syscall. #59520

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d8451f1
FileSystem.Unix: Directory.Delete: remove per item syscall.
tmds Sep 23, 2021
ead0f47
PR feedback
tmds Nov 18, 2021
599514a
test: use Directory.CreateSymbolicLink
tmds Nov 18, 2021
63953d7
Run test against different Delete APIs
tmds Nov 22, 2021
edb54b2
test: update Exists check for Windows
tmds Nov 23, 2021
9014793
Fix tests
tmds Nov 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions src/libraries/System.IO.FileSystem/tests/Directory/Delete.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,35 @@ public void DeletingSymLinkDoesntDeleteTarget()
Assert.False(Directory.Exists(linkPath), "linkPath should no longer exist");
}

[ConditionalFact(nameof(CanCreateSymbolicLinks))]
public void RecursiveDeletingDoesntFollowLinks()
tmds marked this conversation as resolved.
Show resolved Hide resolved
{
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));
tmds marked this conversation as resolved.
Show resolved Hide resolved

// 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()
{
Expand Down
111 changes: 64 additions & 47 deletions src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.IO.Enumeration;
using System.Text;
using Microsoft.Win32.SafeHandles;

Expand Down Expand Up @@ -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);
tmds marked this conversation as resolved.
Show resolved Hide resolved
}
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))
tmds marked this conversation as resolved.
Show resolved Hide resolved
{
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);
tmds marked this conversation as resolved.
Show resolved Hide resolved
}
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)
Expand All @@ -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;
}
tmds marked this conversation as resolved.
Show resolved Hide resolved
goto default;
case Interop.Error.ENOTDIR:
if (topLevel)
{
if (!DirectoryExists(fullPath))
tmds marked this conversation as resolved.
Show resolved Hide resolved
{
throw Interop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(), fullPath, isDirectory: true);
Copy link
Member Author

@tmds tmds Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I kept ENOENT for compatibility. ENOTDIR would be more appropriate since something exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamsitnik I would prefer to change this error code because I think the previous behavior was unintentional.
If you try to delete a socket file using Directory.Delete, the code used to throw ENOENT.
I think it is more appropriate to throw ENOTDIR. Do you agree?

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamsitnik can you share your opinion?

}

// 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;
}

/// <summary>Determines whether the specified directory name should be ignored.</summary>
Expand Down