revert "improve plugins discovery performance"

[crazy-max]

fixes #4145
related to #4142 (comment)

- What I did

Spawning a goroutine for each iteration in the loop when listing plugins is racy unfortunately. plugins slice is protected with a mutex so not sure why it fails.

I tried using a channel to collect the plugins instead of a slice to guarantee that they will be appended to the list in the order they are processed but no dice.

I also tried without errgroup package and simply use sync.WaitGroup but same. I have also created an extra channel to receive errors from the goroutines but racy too.

I think the change in this function is not related to the race condition but newPlugin func is.

- How I did it

Reverts commit 62f2358.

- How to verify it

Run repro from #4145

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

[crazy-max]

Performance improvements from #4129 do not have an impact with this commit. See updated bench result: https://github.com/crazy-max/docker-cli-bench/blob/c53a2c6eb9216af09034205d156c78c222f2db73/bench.md

[cpuguy83]

Looks like its racing because cmd.Commands() modifies cmd

Wrote a test case for this (sort of...) and it looks like its racing because cmd.Commands() is modifying cmd.
Possibily caching Commands() in the main goroutine will fix the issue.... though newPlugin will need to be modified to take a []string instead.

func (c *Command) Commands() []*Command {
	// do not sort commands if it already sorted or sorting was disabled
	if EnableCommandSorting && !c.commandsAreSorted {
		sort.Sort(commandSorterByName(c.commands))
		c.commandsAreSorted = true
	}
	return c.commands
}

[crazy-max]

Looks like its racing because cmd.Commands() modifies cmd

Thanks @cpuguy83 I was just saying that to @thaJeztah on Slack earlier, should have put that in the PR description 🙈

thaJeztah: Thank you! Do we understand what exactly causes the issue? (Would there be ways to work around it?)

Me: I think it could be related to newPlugin being racy. Didn't dig deep about why but that's my best bet. Probably when iterating cobra commands:

cli/cli-plugins/manager/plugin.go

Line 66 in de0d30f

for _, cmd := range rootcmd.Commands() {

---

Wrote a test case for this (sort of...) and it looks like its racing because cmd.Commands() is modifying cmd.
Possibily caching Commands() in the main goroutine will fix the issue.... though newPlugin will need to be modified to take a []string instead.

I will take another look thx

[thaJeztah]

@crazy-max do @cpuguy83 you want to include changes in this PR, or as a follow-up? I'm considering doing a new build of 24.0.0-beta.1 , and we could consider merging this PR (revert of that commit), then work on the alternative

[thaJeztah]

LGTM (if we're okay with doing this as first step)

[crazy-max]

I'm fine for a follow-up 👍