Skip to content

cli: Disable help commands to fix test races

Patrick Steinhardt requested to merge pks-cli-help-subcommand-races into master

We have recently started to observe frequent failures in our CI due to racy writes in our subcommand tests:

WARNING: DATA RACE
Write at 0x000005e5d978 by goroutine 1023:
  github.com/urfave/cli/v2.(*Command).setup()
      $GOPATH/pkg/mod/github.com/urfave/cli/v2@v2.25.1/command.go:138 +0xa8e
  github.com/urfave/cli/v2.(*Command).Run()
      $GOPATH/pkg/mod/github.com/urfave/cli/v2@v2.25.1/command.go:151 +0xa5
  github.com/urfave/cli/v2.(*Command).Run()
      $GOPATH/pkg/mod/github.com/urfave/cli/v2@v2.25.1/command.go:267 +0x126b
  github.com/urfave/cli/v2.(*App).RunContext()
      $GOPATH/pkg/mod/github.com/urfave/cli/v2@v2.25.1/app.go:332 +0x11b2
  github.com/urfave/cli/v2.(*App).Run()
      $GOPATH/pkg/mod/github.com/urfave/cli/v2@v2.25.1/app.go:309 +0x6e6
  gitlab.com/gitlab-org/gitaly/v15/internal/cli/praefect.TestDatalossSubcommand.func1()
      $GOPATH/cli/praefect/subcmd_dataloss_test.go:198 +0x531
  testing.tRunner()
      /usr/share/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/share/go/src/testing/testing.go:1493 +0x47

Previous write at 0x000005e5d978 by goroutine 1021:
  github.com/urfave/cli/v2.(*Command).setup()
      $GOPATH/pkg/mod/github.com/urfave/cli/v2@v2.25.1/command.go:138 +0xa8e
  github.com/urfave/cli/v2.(*Command).Run()
      $GOPATH/pkg/mod/github.com/urfave/cli/v2@v2.25.1/command.go:151 +0xa5
  github.com/urfave/cli/v2.(*Command).Run()
      $GOPATH/pkg/mod/github.com/urfave/cli/v2@v2.25.1/command.go:267 +0x126b
  github.com/urfave/cli/v2.(*App).RunContext()
      $GOPATH/pkg/mod/github.com/urfave/cli/v2@v2.25.1/app.go:332 +0x11b2
  github.com/urfave/cli/v2.(*App).Run()
      $GOPATH/pkg/mod/github.com/urfave/cli/v2@v2.25.1/app.go:309 +0x59d
  gitlab.com/gitlab-org/gitaly/v15/internal/cli/praefect.TestAcceptDatalossSubcommand.func4()
      $GOPATH/cli/praefect/subcmd_accept_dataloss_test.go:156 +0x3f4
  testing.tRunner()
      /usr/share/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/share/go/src/testing/testing.go:1493 +0x47

The root cause of this is that the urfave/cli library adds a "help" subcommand to the app, commands and subcommands when none is provided by the developer already. This "help" subcommand is a pointer stored in a global variable, so all of those structures would point to the same "help" command. But as the urfave/cli library actually modifies all subcommands in order propagate some options down from the top-level application to the commands and subcommands the result is that we have racy writes as soon as we run apps concurrently that have a fallback "help" subcommand.

The urfave/cli library provides two default ways to access help by either calling command subcommand help or by calling command subcommand --help. The latter is likely the more common way to invoke the help menu, and this is not the one that is causing the racy access. So let's fix the race by disabling the "help" subcommand in favor of the --help option.

Closes #5104 (closed).

Edited by Patrick Steinhardt

Merge request reports