Skip to content

[Ignore] Fix recursion count bug #802

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

Merged
merged 4 commits into from
Nov 30, 2018
Merged

[Ignore] Fix recursion count bug #802

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c57e863
[Ignore] Fix recursion count bug
Nov 29, 2018
7c36317
[Ignore] Fix up XML comments
Nov 29, 2018
e4d3416
[Ignore] Improve comment
Nov 29, 2018
ae2a77c
Add label to currDepth parameter
rjmholt Nov 30, 2018
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
122 changes: 56 additions & 66 deletions src/PowerShellEditorServices/Workspace/Workspace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,9 @@ public IEnumerable<string> EnumeratePSFiles()
return Enumerable.Empty<string>();
}

return this.RecursivelyEnumerateFiles(WorkspacePath);
var foundFiles = new List<string>();
this.RecursivelyEnumerateFiles(WorkspacePath, ref foundFiles);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref isn't needed here, but I used it as a guide to the reader that this is effectively the result of the method.

return foundFiles;
}

#endregion
Expand All @@ -309,101 +311,89 @@ public IEnumerable<string> EnumeratePSFiles()
/// <returns>
/// All PowerShell files in the recursive directory hierarchy under the given base directory, up to 64 directories deep.
/// </returns>
private IEnumerable<string> RecursivelyEnumerateFiles(string folderPath)
private void RecursivelyEnumerateFiles(string folderPath, ref List<string> foundFiles, int currDepth = 0)
{
var foundFiles = new List<string>();
var dirStack = new Stack<string>();

// Kick the search off with the base directory
dirStack.Push(folderPath);

const int recursionDepthLimit = 64;
while (dirStack.Any())
{
string currDir = dirStack.Pop();

// Look for any PowerShell files in the current directory
foreach (string pattern in s_psFilePatterns)
{
string[] psFiles;
try
{
psFiles = Directory.GetFiles(currDir, pattern, SearchOption.TopDirectoryOnly);
}
catch (DirectoryNotFoundException e)
{
this.logger.WriteHandledException(
$"Could not enumerate files in the path '{currDir}' due to it being an invalid path",
e);

continue;
}
catch (PathTooLongException e)
{
this.logger.WriteHandledException(
$"Could not enumerate files in the path '{currDir}' due to the path being too long",
e);

continue;
}
catch (Exception e) when (e is SecurityException || e is UnauthorizedAccessException)
{
this.logger.WriteHandledException(
$"Could not enumerate files in the path '{currDir}' due to the path not being accessible",
e);

continue;
}

foundFiles.AddRange(psFiles);
}

// Prevent unbounded recursion here
// If we get too deep, keep processing but go no deeper
if (dirStack.Count >= recursionDepthLimit)
{
this.logger.Write(LogLevel.Warning, $"Recursion depth limit hit for path {folderPath}");
continue;
}

// Add the recursive directories to search next
string[] subDirs;
// Look for any PowerShell files in the current directory
foreach (string pattern in s_psFilePatterns)
{
string[] psFiles;
try
{
subDirs = Directory.GetDirectories(currDir);
psFiles = Directory.GetFiles(folderPath, pattern, SearchOption.TopDirectoryOnly);
}
catch (DirectoryNotFoundException e)
{
this.logger.WriteHandledException(
$"Could not enumerate directories in the path '{currDir}' due to it being an invalid path",
$"Could not enumerate files in the path '{folderPath}' due to it being an invalid path",
e);

continue;
}
catch (PathTooLongException e)
{
this.logger.WriteHandledException(
$"Could not enumerate directories in the path '{currDir}' due to the path being too long",
$"Could not enumerate files in the path '{folderPath}' due to the path being too long",
e);

continue;
}
catch (Exception e) when (e is SecurityException || e is UnauthorizedAccessException)
{
this.logger.WriteHandledException(
$"Could not enumerate directories in the path '{currDir}' due to the path not being accessible",
$"Could not enumerate files in the path '{folderPath}' due to the path not being accessible",
e);

continue;
}

foreach (string subDir in subDirs)
{
dirStack.Push(subDir);
}
foundFiles.AddRange(psFiles);
}

return foundFiles;
// Prevent unbounded recursion here
// If we get too deep, keep processing but go no deeper
if (currDepth >= recursionDepthLimit)
{
this.logger.Write(LogLevel.Warning, $"Recursion depth limit hit for path {folderPath}");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This message will be more accurate now

return;
}

// Add the recursive directories to search next
string[] subDirs;
try
{
subDirs = Directory.GetDirectories(folderPath);
}
catch (DirectoryNotFoundException e)
{
this.logger.WriteHandledException(
$"Could not enumerate directories in the path '{folderPath}' due to it being an invalid path",
e);

return;
}
catch (PathTooLongException e)
{
this.logger.WriteHandledException(
$"Could not enumerate directories in the path '{folderPath}' due to the path being too long",
e);

return;
}
catch (Exception e) when (e is SecurityException || e is UnauthorizedAccessException)
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a better suggestion and think this is the right way to do it... but damn these catch statements take up a lot of space.

Copy link
Contributor

Choose a reason for hiding this comment

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

If they start getting too wide I tend to wrap them like so:

catch (Exception e) when (e is SecurityException || 
                          e is UnauthorizedAccessException)

But I don't think this one is too wide.

Copy link
Member

Choose a reason for hiding this comment

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

yeah I don't think it's too wide - I meant the 3 catches right after each other twice in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the repetition, but I think a function call around a lambda just to catch certain exceptions is a rather nasty abuse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Would be more abstractable in a world where exceptions were returned as union types...)

{
this.logger.WriteHandledException(
$"Could not enumerate directories in the path '{folderPath}' due to the path not being accessible",
e);

return;
}

foreach (string subDir in subDirs)
{
RecursivelyEnumerateFiles(subDir, ref foundFiles, currDepth + 1);
Copy link
Contributor Author

@rjmholt rjmholt Nov 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thoughts on currDepth: currDepth + 1 in the last arg?

}
}

/// <summary>
Expand Down