nspcc-dev / neofs-node

NeoFS is a decentralized distributed object storage integrated with the Neo blockchain
https://fs.neo.org
GNU General Public License v3.0
31 stars 38 forks source link

Cobra command runners skip `defer`ed funcs and postruns on failures #2890

Open cthulhu-rider opened 3 weeks ago

cthulhu-rider commented 3 weeks ago

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()
    }
3. `make bin/neofs-test`
4. `./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"
```go
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 commented 3 weeks ago
roman-khimov commented 2 weeks ago

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

carpawell commented 2 weeks ago

RunE's problem (and i agree with it): https://github.com/nspcc-dev/neofs-node/issues/623.

cthulhu-rider commented 2 weeks ago

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

silencers prevent this as in code example