Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.IO.Enumeration;
using System.Runtime.CompilerServices;
using System.Text;
using Microsoft.Win32.SafeHandles;

Expand Down Expand Up @@ -496,6 +497,8 @@ private static void RemoveDirectoryRecursive(string fullPath)
{
if (isDirectory)
{
// Since this is a recursive call, we have to ensure that we have sufficient stack space.
RuntimeHelpers.EnsureSufficientExecutionStack();
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little concerned this is going to start introducing failures that weren't there before. Previously if we recurred too deeply the process would crash with a stack overflow, but EnsureSufficientExecutionStack is fairly conservative about what "sufficient execution stack" means, and it could end up throwing an exception well before the process would otherwise crash. I expect there are some cases today of very nested directories that are working today because they're not deep enough to cause a crash but would start throwing an exception with this because they are deep enough to hit the stack execution depth limit.

Alternatives might include:

  • Switching the whole implementation to be iterative with a manually-managed stack rather than recursive.
  • Only doing so once TryEnsureSufficientExecutionStack returns false.
  • When TryEnsureSufficientExecutionStack returns false, fork off an additional thread to continue the recursive processing.

RemoveDirectoryRecursive(childPath);
}
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Buffers;
using System.ComponentModel;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using Microsoft.Win32.SafeHandles;

Expand Down Expand Up @@ -344,6 +345,8 @@ private static void RemoveDirectoryRecursive(string fullPath, ref Interop.Kernel
// Not a reparse point, or the reparse point isn't a name surrogate, recurse.
try
{
// Since this is a recursive call, we have to ensure that we have sufficient stack space.
RuntimeHelpers.EnsureSufficientExecutionStack();
RemoveDirectoryRecursive(
Path.Combine(fullPath, fileName),
findData: ref findData,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,30 @@ public void RecursiveDelete_DeepNesting()
Delete(rootDirectory, recursive: true);
}

[Fact]
[PlatformSpecific(TestPlatforms.Windows)]
public void RecursiveDelete_issue84344()
{
// Test the scenario described in the GitHub issue #84344 where the nesting depth of subdirectories is huge (14000).
// But this test will take a long time to run if we use 14000 as the depth, because the original depth number is too deep.
// Therefore we reduce the depth to 9000 for quick testing purpose but with a large enough number.
// See https://github.com/dotnet/runtime/issues/84344
// Although the scenario on the original issue is done on Windows,
// we should be able to run this test on all platforms to test the recursive Directory.Delete().
// Unfortunately, on Unix/Linux the max path length is limited to 4096, therefore this test is targeted to only runs on Windows.
DirectoryInfo testDir = new DirectoryInfo(GetTestFilePath());
var depth = 9000;

// Create a specific subdir only for this test.
DirectoryInfo subDir = testDir.CreateSubdirectory("test_issue84344");
for (int i = 0; i < depth; i++)
{
subDir = subDir.CreateSubdirectory("a");
}
// Now delete the subdir recursively.
Directory.Delete(subDir.FullName, recursive: true);
}

[Fact]
[PlatformSpecific(TestPlatforms.Windows)] // Recursive delete throws IOException if directory contains in-use file
public void RecursiveDelete_ShouldThrowIOExceptionIfContainedFileInUse()
Expand Down
Loading