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.

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

Fix hangs when file path is provided with wildcard without recurse flag #2800

Merged
merged 8 commits into from
Mar 15, 2024
Merged
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
4 changes: 4 additions & 0 deletions ReleaseHistory.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
# SARIF Package Release History (SDK, Driver, Converters, and Multitool)

## UNRELEASED
* BUG: Resolve process hangs when a file path is provided with a wildcard, but without a `-r` (recurse) flag during the multi-threaded analysis file enumeration phase.

## **v4.5.4 [Sdk](https://www.nuget.org/packages/Sarif.Sdk/v4.5.4) | [Driver](https://www.nuget.org/packages/Sarif.Driver/v4.5.4) | [Converters](https://www.nuget.org/packages/Sarif.Converters/v4.5.4) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/v4.5.4) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/v4.5.4)
* BUG: Fix incorrect base class in rule ADO2012.

Expand Down
27 changes: 12 additions & 15 deletions src/Sarif.Driver/OrderedFileSpecifier.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public OrderedFileSpecifier(string specifier,
FileSystem = fileSystem ?? Sarif.FileSystem.Instance;
}

private const int ChannelCapacity = 10 * 1024; // max ~2.5 MB memory given 256 char max path length.
internal const int ChannelCapacity = 10 * 1024; // max ~2.5 MB memory given 256 char max path length.
private readonly bool recurse;
private readonly string specifier;
private readonly long maxFileSizeInKilobytes;
Expand Down Expand Up @@ -87,27 +87,24 @@ private IEnumerable<IEnumeratedArtifact> EnumeratedArtifacts()

Task directoryEnumerationTask;

if (this.recurse)
directoryEnumerationTask = Task.Run(() =>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue and fix is simple, in the recurse false code path we missed to use Task.Run to run the file enumeration operation asynchronously on the separate thread.
Now we merged in a same code block so both
recurse true and false will have it.

{
directoryEnumerationTask = Task.Run(() =>
try
{
try
if (this.recurse)
{
EnqueueAllFilesUnderDirectory(directory, filesToProcessChannel.Writer, filter, new SortedSet<string>(StringComparer.Ordinal));
}
finally
else
{
filesToProcessChannel.Writer.Complete();
WriteFilesInDirectoryToChannel(directory, filesToProcessChannel, filter, new SortedSet<string>(StringComparer.Ordinal));
}
});
}
else
{
WriteFilesInDirectoryToChannel(directory, filesToProcessChannel, filter, new SortedSet<string>(StringComparer.Ordinal));

filesToProcessChannel.Writer.Complete();
directoryEnumerationTask = Task.CompletedTask;
}
}
finally
{
filesToProcessChannel.Writer.Complete();
}
});

ChannelReader<string> reader = filesToProcessChannel.Reader;
while (!reader.Completion.IsCompleted)
Expand Down
64 changes: 64 additions & 0 deletions src/Test.FunctionalTests.Sarif/MultitoolCommandLineTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
using System.IO;

using FluentAssertions;
using FluentAssertions.Execution;

using Microsoft.CodeAnalysis.Sarif.Driver;

using Xunit;

Expand Down Expand Up @@ -35,5 +38,66 @@ public void Multitool_LaunchesAndRunsSuccessfully()
process.ExitCode.Should().Be(0);
}
}

#if DEBUG
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to test the analyze command hang, in sdk we only have analyze-test which is also wrapped under #if DEBUG so my test will also need to be wrapped under it.
I have tested .\BuildAndTest.cmd -Configuration Debug will all pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the 12k files were a red herring, and it would have hung on one file. If that's true, then I don't think we shouldn't take a 12k/5 sec UT when an instantaenous one will do.

What do you think?

Also: did you go back and verify that the UT hangs on the unmodified code? I think that's important for this tes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was confused. Shaopeng educated me offline. Issue is we fill the channel without yielding our thread. So you really do need 10k+ files, because that's the channel size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also: did you go back and verify that the UT hangs on the unmodified code? I think that's important for this tes.
----- yes, undo the fix and leave test can repro and have the error in the screenshot in the other comment.

as talk in IM we need 10241 files.

Will work on the change you recommended in IM.

[Fact]
[Trait(TestTraits.WindowsOnly, "true")]
public void Multitool_LaunchesAndRunsSuccessfully_WithNumberOfFilesExceedingChannelCapacity()
{
using var assertionScope = new AssertionScope();

string multitoolPath = Path.GetFullPath(Path.Combine(Directory.GetCurrentDirectory(),
@"..\..\Sarif.Multitool\netcoreapp3.1\Sarif.Multitool.exe"));

string directoryPath = Path.Combine(Path.GetTempPath(), "SarifMultitoolTestFilesWithNumberOfFilesExceedingChannelCapacity");

if (Directory.Exists(directoryPath))
{
Directory.Delete(directoryPath, true);
}

Directory.CreateDirectory(directoryPath);

int fileCount = OrderedFileSpecifier.ChannelCapacity + 1;

for (int i = 1; i <= fileCount; i++)
{
string filename = $"file_{i}.txt";
string filepath = Path.Combine(directoryPath, filename);
File.WriteAllText(filepath, " ");
}

var startInfo = new ProcessStartInfo(multitoolPath, $@"analyze-test {directoryPath}\*")
{
WindowStyle = ProcessWindowStyle.Hidden,
CreateNoWindow = true,
RedirectStandardOutput = true,
UseShellExecute = false
};

using (var process = Process.Start(startInfo))
{
var timer = new System.Timers.Timer(30000); // 30 seconds.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with fix, it will pass, it will only runs within 5 seconds so will not affect testing run total time:
image

before fix, it will fail and the message will be like below for dev:
image

timer.Elapsed += (sender, e) =>
{
if (!process.HasExited)
{
process.Kill();
}
};
timer.Start();
string output = process.StandardOutput.ReadToEnd();
process.WaitForExit();
timer.Stop();
process.ExitCode.Should().Be(0);
output.Should().Contain(
$"Done. {fileCount:n0} files scanned.",
$"analyzing {fileCount:n0} small files should not result in freezing and should finish within 30 seconds, " +
"typically completing in just 5 seconds");
}

Directory.Delete(directoryPath, true);
}
#endif
}
}
Loading