execute az cli commands async #3286
Conversation
| } | ||
|
|
||
| public int RunAzCli(string arguments, out string? stdOut, out string? stdErr) | ||
| public async Task<(int ExitCode, string? StdOut, string? StdErr)> RunAzCliAsync(string arguments) |
There was a problem hiding this comment.
Pass a cancellation token and link in Task.WhenAll below?
| AspireCommandService aspireCommandService = new(builder); | ||
|
|
||
| //aspire command adding does not need to be async | ||
| aspireCommandService.AddScaffolderCommands(); |
There was a problem hiding this comment.
Why are we adding ScaffolderComamnds twice. on L28 and L32
There was a problem hiding this comment.
one is aspire and the other is aspnet commands, they contain different scaffolders
| Required = true, | ||
| PickerType = InteractivePickerType.CustomPicker, | ||
| CustomPickerValues = _usernames | ||
| CustomPickerValues = _azureInformation!.Usernames |
There was a problem hiding this comment.
It doesn't appear to be safe to suppress these null checks. The previous code would have a default empty collection here if getting Azure info failed. This will have null, and this will throw. Consider something like:
| CustomPickerValues = _azureInformation!.Usernames | |
| CustomPickerValues = _azureInformation?.Usernames ?? [] |
Ditto below two more times.
| var result = StringUtil.ConvertStringToArray(stdOut); | ||
| if (result.Length is 0) |
There was a problem hiding this comment.
Unrelated to this PR, but this seems to allocate an array when all it really wants to know is that there was at least one value. If stdOut is huge and there are many items, this might be wasteful.
| // Wait for process to exit with a timeout (e.g., 30 seconds) or cancellation | ||
| const int timeoutMilliseconds = 30000; | ||
| bool exited = process.WaitForExit(timeoutMilliseconds); | ||
| Task<bool> waitForExitTask = Task.Run(() => process.WaitForExit(timeoutMilliseconds), cancellationToken); |
There was a problem hiding this comment.
Can this use WaitForExitAsync instead, and simplify the code below a little? It's .NET 5+ (not sure what targets this needs to run on).
Could make an extensions method for this. Something like:
public static async Task<bool> WaitForExitAsync(
this Process process,
TimeSpan timeout,
CancellationToken cancellationToken = default)
{
using var timeoutCts = new CancellationTokenSource(timeout);
using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCts.Token);
try
{
await process.WaitForExitAsync(linkedCts.Token);
return !timeoutCts.IsCancellationRequested;
}
catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested)
{
// Timed out.
return false;
}
}Another option would be to register a callback on the cancellation token that performs the kill.
| //ignore | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
It's not obvious to me why this is safe to ignore. Can it be described in the comment?
AzureInformationrecordazCLI commands runasyncpart of work for #3249