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

SuggestionStore::GetCompletions to check exit code of invoked applica… #2160

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

baradgur
Copy link

This fixes #2137.

@baradgur
Copy link
Author

@dotnet-policy-service agree

@@ -44,7 +44,7 @@ public string GetCompletions(string exeFileName, string suggestionTargetArgument

Task<string> readToEndTask = process.StandardOutput.ReadToEndAsync();

if (readToEndTask.Wait(timeout))
if (readToEndTask.Wait(timeout) && process.HasExited && process.ExitCode == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@@ -44,7 +44,7 @@ public string GetCompletions(string exeFileName, string suggestionTargetArgument

Task<string> readToEndTask = process.StandardOutput.ReadToEndAsync();

if (readToEndTask.Wait(timeout))
if (readToEndTask.Wait(timeout) && process.HasExited && process.ExitCode == 0)

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, 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.

@baradgur
Copy link
Author

baradgur commented Apr 12, 2023

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")]
Copy link
Author

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.

Copy link
Author

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.

{
result = readToEndTask.Result;
result = process.StandardOutput.ReadToEnd();

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);
Copy link

@KalleOlaviNiemitalo KalleOlaviNiemitalo Apr 14, 2023

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 the stringBuilder.ToString() call; although the child process has exited, grandchild processes might still write to the pipe.

Copy link
Author

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
Copy link
Author

baradgur commented Apr 16, 2023

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.
Locally all tests work.

@jonsequitur
Copy link
Contributor

@baradgur Here's the output from the failing test. I've triggered another run to see if it's consistent.

image

@baradgur
Copy link
Author

baradgur commented Jun 7, 2023

@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.
My current plan is to configure gihub actions on a separate branch (or even repo, not sude wheter github allows actions on a fork) to run tests on the machine with the same configuration as the build machine (e.g ubuntu one) and add some robust logging to diagnose the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetCompletions should check exit code of invoked application
3 participants