Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cobra command runners skip defered funcs and postruns on failures #2890

Open
cthulhu-rider opened this issue Jul 10, 2024 · 4 comments
Open
Labels
bug Something isn't working good first issue Good for newcomers I4 No visible changes neofs-cli NeoFS CLI application issues neofs-lens NeoFS Lens app issues S3 Minimally significant U4 Nothing urgent
Milestone

Comments

@cthulhu-rider
Copy link
Contributor

cthulhu-rider commented Jul 10, 2024

some Cobra-based programs exit instantly on particular errors:

they call os.Exit due to which neither cmd postruns, nor finalizers, nor even exec defer funcs are called. The latter are usually responsible for allocating resources such as network connections, files, etc.

Steps to Reproduce

  1. add cmd/neofs-test/main.go
func main() {
	(&cobra.Command{
		Use: "test",
		Run: func(cmd *cobra.Command, _ []string) {
			cobra.OnFinalize(func() {
				cmd.Println("in finalizer")
			})
			defer func() {
				cmd.Println("in defer")
			}()
			fmt.Fprintf(os.Stderr, "err: %v\n", errors.New("any error"))
			os.Exit(1)
		},
		PostRun: func(cmd *cobra.Command, args []string) {
			cmd.Println("in PostRun")
		},
		PersistentPostRun: func(cmd *cobra.Command, args []string) {
			cmd.Println("in PersistentPostRun")
		},
	}).Execute()
}
  1. make bin/neofs-test
  2. `./bin/neofs-test

Expected Behavior

$ ./bin/neofs-test
in defer
in PostRun
in PersistentPostRun
in finalizer
err: any error
$ echo $?
1

Current Behavior

$ ./bin/neofs-test
err: any error
$ echo $?
1

Possible Solution

1. Finalizing closure in Run

the most clean solution to me in terms of code and command lifetime. At the same time, requires a lot of changes, code will be pretty "spaghetti"

func main() {
	err := (&cobra.Command{
		Use:           "test",
		SilenceErrors: true,
		SilenceUsage:  true,
		Run: func(cmd *cobra.Command, _ []string) {
			var code int
			var err error
			cobra.OnFinalize(func() {
				cmd.Println("in finalizer")
				if err != nil {
					fmt.Fprintln(os.Stderr, "err:", err)
				}
				os.Exit(code)
			})
			defer func() {
				cmd.Println("in defer")
			}()
			code = 2
			err = errors.New("any error")
		},
		PostRun: func(cmd *cobra.Command, args []string) {
			cmd.Println("in PostRun")
		},
		PersistentPostRun: func(cmd *cobra.Command, args []string) {
			cmd.Println("in PersistentPostRun")
		},
	}).Execute()
	if err != nil {
		fmt.Fprintln(os.Stderr, err)
		os.Exit(1)
	}
}

2. Specific error + RunE + unsupported *PostRun

the most correct solution to me in terms of os.Exit, but narrows the breadth of cmd lifetime use. Also requires a lot of changes but code will look less "spaghetti"

type ExitErr struct {
	Code  int
	Cause error
}

func (x ExitErr) Error() string { return x.Cause.Error() }

func main() {
	err := (&cobra.Command{
		Use:           "test",
		SilenceErrors: true,
		SilenceUsage:  true,
		RunE: func(cmd *cobra.Command, _ []string) error {
			cobra.OnFinalize(func() {
				cmd.Println("in finalizer")
			})
			defer func() {
				cmd.Println("in defer")
			}()
			return ExitErr{Code: 2, Cause: fmt.Errorf("err: %w", errors.New("any error"))}
		},
	}).Execute()
	if err != nil {
		var e ExitErr
		if !errors.As(err, &e) {
			e.Code = 1
		}
		fmt.Fprintln(os.Stderr, err)
		os.Exit(e.Code)
	}
}

3. Fake defer's + unsupported *PostRun

the worst to me but fast solution requiring less changes

var deferred []func()

func Defer(f func()) { deferred = append(deferred, f) }

func ExitOnErr(cmd *cobra.Command, errFmt string, err error) {
	...
	cmd.PrintErrln(err)
	for i := range deferred {
		deferred[len(deferred)-i-1]()
	}
	os.Exit(code)
}

Context

Cobra itself has no in-box mechanism to work with OS exit codes, only similar funcs

ive been hiding this topic in myself for a long time, but now im ready to present it

Regression

no

Your Environment

@cthulhu-rider cthulhu-rider added bug Something isn't working neofs-cli NeoFS CLI application issues neofs-lens NeoFS Lens app issues good first issue Good for newcomers labels Jul 10, 2024
@cthulhu-rider
Copy link
Contributor Author

@roman-khimov
Copy link
Member

I'd just use RunE and move ExitOnErr code-related logic to main.

@roman-khimov roman-khimov added U4 Nothing urgent S3 Minimally significant I4 No visible changes labels Jul 16, 2024
@carpawell
Copy link
Member

RunE's problem (and i agree with it): #623.

@cthulhu-rider
Copy link
Contributor Author

RunE's problem (and i agree with it): #623.

silencers prevent this as in code example

End-rey added a commit that referenced this issue Sep 19, 2024
Use `RunE` instead of `Run` and throw an error to main function. Then handle
error with custom error handler.
Update CHANGELOG.md.

Closes #2890, #623.

Signed-off-by: Andrey Butusov <[email protected]>
End-rey added a commit that referenced this issue Sep 19, 2024
Use `RunE` instead of `Run` and throw an error to main function. Then handle
error with custom error handler.
Update CHANGELOG.md.

Closes #2890, #623.

Signed-off-by: Andrey Butusov <[email protected]>
@roman-khimov roman-khimov added this to the v0.44.0 milestone Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers I4 No visible changes neofs-cli NeoFS CLI application issues neofs-lens NeoFS Lens app issues S3 Minimally significant U4 Nothing urgent
Projects
None yet
Development

No branches or pull requests

3 participants