Default app design encourages data race #1290
Comments
|
I hit something similar while working on MichaelMure/git-bug#531 (comment) Running the code below often gives a Should flagCompletionFunctions be a member of package main
import (
"fmt"
"sync"
"github.com/spf13/cobra"
)
func main() {
cmd := cobra.Command{}
N := 2
options := make([]string, N)
var wg sync.WaitGroup
for i := 0; i < N; i++ {
flag := fmt.Sprintf("flag%d", i)
cmd.Flags().StringVarP(&options[i], flag, "", "value", "usage")
wg.Add(1)
go func() {
defer wg.Done()
err := cmd.RegisterFlagCompletionFunc(flag, nil)
if err != nil {
panic(err)
}
}()
}
wg.Wait()
} |
|
This issue is being marked as stale due to a long period of inactivity |
|
I also dont use the pattern given by the default app design (init method defining flags with global variables backing them) and instead opt to have each method define a closure around the values it needs so each command has its own version. This is also really helpful when testing code since you can more easily test individual methods without worrying about flags/commands interacting during the unit tests. I'm a new maintainer so I'm not sure how likely it is to get that default design redone or what the arguments for it are but I think its reasonable to ask. |
johnSchnake
added
area/cobra-command
jpmcb
added
lifecycle/needs-pr
|
it happens on my side during testing the same command with different opts (running them all fails most of them) but they work in isolation. so i guess its a global state problem as mentioned in this thread. i solved it with this pattern (as described above): //main.go
func main() {
rootCmd := cmd.NewRootCommand()
cmd.AddChildCommands(rootCmd)
cmd.Execute(rootCmd)
}//cmd/root.go
func Execute(command *cobra.Command) {
err := command.Execute()
if err != nil {
os.Exit(1)
}
}
func NewRootCommand() *cobra.Command {
var rootCmd = &cobra.Command{
}
return rootCmd
}
func AddChildCommands(rootCmd *cobra.Command) {
NewApplyCommand(rootCmd)
}//cmd/child.go
func NewApplyCommand(root *cobra.Command) {
var applyCmd = &cobra.Command{
}
root.AddCommand(applyCmd)
}Tests are now isolated nicely. Thanks for the hint @apapsch |
|
Going to simply link this into the spf13/cobra-cli/issues/ so that when it gets redone we ensure the data races are gone. Closing as duplicated/moved there. |
Using the cobra skeleton generator,
root.gocontains a global variablecfgFile, and the pointer is passed torootCmd.PersistentFlags().StringVar(). This may result in data race in some circumstances. In my case it happened withgo test -race -vand a table based test containing three sub tests.I worked around the problem by removing
cfgFile,func initConfigandfunc initfromroot.go. CLI flags suffice for me, so that fits the bill.The snake could be defanged by removing the need for global variables and
func inits. In my app I replacedrootCmdand the other*cobra.Commandglobals with NewXXXCommand functions:This requires
mainto grow a bit:It improves testability, because I can construct the commands in my test file without worrying about global state scattered across a few files.
This approach has the downside that
mainneeds to grow with each command, because NewXXXCommand adds the child command to the parent. One might argue, adding child to parent needs to happen anyway, and it is better to do it in one function than scatter it across manyinitfunctions.The text was updated successfully, but these errors were encountered: