SuggestionStore::GetCompletions to check exit code of invoked applica…#2160
SuggestionStore::GetCompletions to check exit code of invoked applica…#2160baradgur wants to merge 6 commits intodotnet:mainfrom
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.
The else 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.
I'm not sure this is entirely reliable. If the child process first closes its standard output and then exits, then readToEndTask.Wait might complete while process.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.
|
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 |
|
|
||
| namespace System.CommandLine.Suggest.Tests; | ||
|
|
||
| [Collection("TestsWithTestApps")] |
There was a problem hiding this comment.
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.
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.
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); |
There was a problem hiding this comment.
Should be just Append, not AppendLine.
There may be a thread safety problem if there is still more data coming in during the stringBuilder.ToString() call; although the child process has exited, grandchild processes might still write to the pipe.
There was a problem hiding this comment.
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.
|
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. |
3 participants
This fixes #2137.