- Notifications
You must be signed in to change notification settings - Fork412
SuggestionStore::GetCompletions to check exit code of invoked applica…#2160
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@dotnet-policy-service agree |
| Task<string>readToEndTask=process.StandardOutput.ReadToEndAsync(); | ||
| if(readToEndTask.Wait(timeout)) | ||
| if(readToEndTask.Wait(timeout)&&process.HasExited&&process.ExitCode==0) |
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.
Theelse block might need some additional checks as well. It doesn't appear that we have code coverage over both of these code branches.
| Task<string>readToEndTask=process.StandardOutput.ReadToEndAsync(); | ||
| if(readToEndTask.Wait(timeout)) | ||
| if(readToEndTask.Wait(timeout)&&process.HasExited&&process.ExitCode==0) |
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.
I'm not sure this is entirely reliable. If the child process first closes its standard output and then exits, thenreadToEndTask.Wait might complete whileprocess.HasExited is still false. And maybe that can even happen if the child process does not intentionally delay exiting. It might be more reliable to also call Process.WaitForExit(TimeSpan) with a small grace period.
baradgur commentedApr 12, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I saw the issue yesterday and that is why th PR is not marked as ready for review yet. Sorry for confusion, I will fix and test this today. Thanks for the feedback |
| namespaceSystem.CommandLine.Suggest.Tests; | ||
| [Collection("TestsWithTestApps")] |
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.
Note: this attribute is inherited, so both SuggestionStoreTests and DotnetSuggestEndToEndTests will run in the same collection, meaning - not in parallel. This allows the gracefull cleanup of TestsApp files.
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.
Not sure whether that solution with the faulted console app was the right way, honestly.
| if(process.WaitForExit(timeout)&&process.ExitCode==0) | ||
| { | ||
| result=readToEndTask.Result; | ||
| result=process.StandardOutput.ReadToEnd(); |
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.
This might wait beyond the timeout, if the child process exited but a grandchild process is still holding the pipe open.
| { | ||
| if(eventArgs.Data!=null) | ||
| { | ||
| stringBuilder.AppendLine(eventArgs.Data); |
KalleOlaviNiemitaloApr 14, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Should be just Append, not AppendLine.
There may be a thread safety problem if there is still more data coming in during thestringBuilder.ToString() call; although the child process has exited, grandchild processes might still write to the pipe.
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.
Thank you, might be the mistake is here! I hope it is my last. Otherwise I am completely at loss right now on why the test keeps faling.
Regarding Append/AppendLine: seems not the case here - I have tested it, the eventArgs.Data comes without any line terminator.
baradgur commentedApr 16, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Can I do a diagnostic commit (e.g. add some messages) and revert it later? Is this allowed? The test is failing and I unfortunately have no means to identify the reason. |
@baradgur Here's the output from the failing test. I've triggered another run to see if it's consistent.
|
@jonsequitur Thanks, I saw this one :) My trouble is - I cannot reproduce the issue on my side no matter what I try. If you can offer some thoughts about this, I would be grateful. |
Thisfixes#2137.