Default app design encourages data race

[apapsch]

Using the cobra skeleton generator, root.go contains a global variable cfgFile, and the pointer is passed to rootCmd.PersistentFlags().StringVar(). This may result in data race in some circumstances. In my case it happened with go test -race -v and a table based test containing three sub tests.

=== RUN   TestExecution
=== RUN   TestExecution/successful_date
==================
WARNING: DATA RACE
Write at 0x000001513440 by goroutine 13:
  github.com/spf13/pflag.newStringValue()
      /home/ep/go/pkg/mod/github.com/spf13/pflag@v1.0.5/string.go:7 +0x44
  github.com/spf13/pflag.(*FlagSet).StringVar()
      /home/ep/go/pkg/mod/github.com/spf13/pflag@v1.0.5/string.go:37 +0x36
  gitlab.com/sterndata/cronjobd/cmd.NewRootCommand()
      /home/ep/src/monolith/go/cronjobd/cmd/root.go:29 +0x187
  gitlab.com/sterndata/cronjobd_test.TestExecution.func2()
      /home/ep/src/monolith/go/cronjobd/integration_test.go:62 +0x40e
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1123 +0x202
[...]
[...]
==================
==================
WARNING: DATA RACE
Write at 0x00c00015c710 by goroutine 13:
  github.com/spf13/viper.(*Viper).SetConfigName()
      /home/ep/go/pkg/mod/github.com/spf13/viper@v1.7.1/viper.go:1931 +0x389
  github.com/spf13/viper.SetConfigName()
      /home/ep/go/pkg/mod/github.com/spf13/viper@v1.7.1/viper.go:1928 +0x357
  gitlab.com/sterndata/cronjobd/cmd.initConfig()
      /home/ep/src/monolith/go/cronjobd/cmd/root.go:62 +0x356
  github.com/spf13/cobra.(*Command).preRun()
      /home/ep/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:880 +0x81
  github.com/spf13/cobra.(*Command).execute()
      /home/ep/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:816 +0x1b5
  github.com/spf13/cobra.(*Command).ExecuteC()
      /home/ep/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:958 +0x4b2
  github.com/spf13/cobra.(*Command).ExecuteC()
      /home/ep/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:907 +0xaa8
  github.com/spf13/cobra.(*Command).Execute()
      /home/ep/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:895 +0x4d7
  github.com/spf13/cobra.(*Command).ExecuteContext()
      /home/ep/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:888 +0x4ce
  gitlab.com/sterndata/cronjobd_test.TestExecution.func2()
      /home/ep/src/monolith/go/cronjobd/integration_test.go:65 +0x48d
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1123 +0x202

I worked around the problem by removing cfgFile, func initConfig and func init from root.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 replaced rootCmd and the other *cobra.Command globals with NewXXXCommand functions:

func NewRootCommand() *cobra.Command {
	return &cobra.Command
                // ...
	}
}
func NewServeCommand(parent *cobra.Command) *cobra.Command {
	cmd := &cobra.Command{
		// ...
	}
	parent.AddCommand(cmd)
	// cmd.Flags().String(...)
	return cmd
}

This requires main to grow a bit:

func main() {
	rootCmd := cmd.NewRootCommand()
	cmd.NewRunCommand(rootCmd)
	cmd.NewServeCommand(rootCmd)
	cmd.Execute(rootCmd)
}

It improves testability, because I can construct the commands in my test file without worrying about global state scattered across a few files.

func TestExecution(t *testing.T) {
	rootCmdForServe := cmd.NewRootCommand()
	rootCmdForServe.SetArgs([]string{"serve"})
	rootCmdForServe.SetErr(ioutil.Discard)
	serveCmd := cmd.NewServeCommand(rootCmdForServe)
	go func() {
		err := serveCmd.ExecuteContext(...)
		if err != nil {
			t.Fatalf("Failed executing serve command: %v", err)
		}
	}()
        // ...
}

This approach has the downside that main needs 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 many init functions.

[krobelus]

I hit something similar while working on git-bug/git-bug#531 (comment)

Running the code below often gives a fatal error: concurrent map writes.

Should flagCompletionFunctions be a member of Command? Should it be guarded by a lock? Or should we explicitly forbid creating commands from different Goroutines?

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()

}

[github-actions]

This issue is being marked as stale due to a long period of inactivity

[johnSchnake]

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.

[digitalkaoz]

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