-
Notifications
You must be signed in to change notification settings - Fork 92
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
Changes from 7 commits
3d0f760
59b08fe
48cedef
9b07e63
6850ec2
20dee60
fd835d6
8df8fe2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
using System.IO; | ||
|
||
using FluentAssertions; | ||
using FluentAssertions.Execution; | ||
|
||
using Xunit; | ||
|
||
|
@@ -35,5 +36,64 @@ public void Multitool_LaunchesAndRunsSuccessfully() | |
process.ExitCode.Should().Be(0); | ||
} | ||
} | ||
|
||
#if DEBUG | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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_With12kFiles() | ||
{ | ||
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(), "SarifMultitool12kTestFiles"); | ||
|
||
if (Directory.Exists(directoryPath)) | ||
{ | ||
Directory.Delete(directoryPath, true); | ||
} | ||
|
||
Directory.CreateDirectory(directoryPath); | ||
|
||
for (int i = 1; i <= 12000; i++) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Soft-recommend (do not require) using internalsvisibleto to couple this parameter to the channel size. Signing off. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated to use the ChannelCapacity + 1. |
||
{ | ||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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. 12,000 files scanned.", | ||
"analyzing 12,000 small files should not result in freezing and should finish within 30 seconds, " + | ||
"typically completing in just 5 seconds"); | ||
} | ||
|
||
Directory.Delete(directoryPath, true); | ||
} | ||
#endif | ||
} | ||
} |
There was a problem hiding this comment.
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.