Fix deadlock and ux bugs in asoctl #4270
Conversation
theunrepentantgeek
requested review from
davefellows,
matthchr,
babbageclunk and
super-harsh
as code owners
| "github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud" | ||
| "github.com/Azure/azure-sdk-for-go/sdk/azidentity" | ||
| "github.com/pkg/errors" | ||
| "github.com/spf13/cobra" | ||
| "os" |
There was a problem hiding this comment.
minor: avoid reordering these? stdlib imports in first group makes sense IMO.
| @@ -128,6 +122,9 @@ func importAzureResource( | |||
| return errors.Wrapf(err, "failed to create ARM client") | |||
| } | |||
|
|
|||
| // | |||
| @@ -139,6 +136,9 @@ func importAzureResource( | |||
| } | |||
| } | |||
|
|
|||
| // Make sure all output is written when we're done | |||
There was a problem hiding this comment.
minor: maybe leave a note that this shouldn't be defered until right before Import is called, because the Import call is what actually uses/writes to the progress bar and calls to Wait will deadlock if a bar was added but never written to?
Though... if that's actually the case, is it also possible to deadlock if armIDs is empty? We'd not take the for loop at all but importreporter.NewBar would still be created which is what actually triggers the deadlock?
There was a problem hiding this comment.
Is it possible to test this case somehow?
There was a problem hiding this comment.
As long as one bar is created, nothing deadlocks. The call to importer.Import() will do that, so we should be safe. I'll refactor the code to tidy things up and add some docs.
What this PR does / why we need it:
Fixes a deadlock bug that occurs if an error is returned before the first progressbar is created.
Fixes a UX bug where completed progress bars are left on screen as debris.
How does this PR make you feel:
