Fix hangs when file path is provided with wildcard without recurse flag #2800
Conversation
…hout `-r` (recurse) flag during multithreaded analysis file enumeration phase.
shaopeng-gh
requested review from
michaelcfanning,
suvamM,
scottoneil-ms,
rwoll,
yongyan-gh,
LingZhou-gh,
EasyRhinoMSFT and
cfaucon
as code owners
|
|
||
| filesToProcessChannel.Writer.Complete(); | ||
| directoryEnumerationTask = Task.CompletedTask; | ||
| directoryEnumerationTask = Task.Run(() => |
There was a problem hiding this comment.
where is the test for this --- working on it next. Want to give an early update that the issue has been found, or else will need Scott's view since he made this change.
| @@ -35,5 +36,64 @@ public void Multitool_LaunchesAndRunsSuccessfully() | |||
| process.ExitCode.Should().Be(0); | |||
| } | |||
| } | |||
|
|
|||
| #if DEBUG | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
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.
|
|
||
| using (var process = Process.Start(startInfo)) | ||
| { | ||
| var timer = new System.Timers.Timer(30000); // 30 seconds. |
There was a problem hiding this comment.
with fix, it will pass, it will only runs within 5 seconds so will not affect testing run total time:
before fix, it will fail and the message will be like below for dev:
| @@ -87,27 +87,24 @@ private IEnumerable<IEnumeratedArtifact> EnumeratedArtifacts() | |||
|
|
|||
| Task directoryEnumerationTask; | |||
|
|
|||
| if (this.recurse) | |||
| directoryEnumerationTask = Task.Run(() => | |||
There was a problem hiding this comment.
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.
|
|
||
| Directory.CreateDirectory(directoryPath); | ||
|
|
||
| for (int i = 1; i <= 12000; i++) |
There was a problem hiding this comment.
Soft-recommend (do not require) using internalsvisibleto to couple this parameter to the channel size.
ChannelCapacity.
Signing off.
There was a problem hiding this comment.
updated to use the ChannelCapacity + 1.
Tried a few times, only need + 1 to repro.
-r(recurse) flag during the multithreaded analysis file enumeration phase.