Cli: Backport recover() logic from v2 to v1

[mponton]

I just spent over half an over trying to figure out why the hell my app quit without any error message or panic. Then I found this code in app.go:

    defer func() {
        if r := recover(); r != nil {
            switch r.(type) {
            case error:
                err = r.(error)
            default:
                err = NewExitError(fmt.Sprintf("ERROR unknown Action error: %v. See %s", r, appActionDeprecationURL), 2)
            }
        }
    }()

This code silently swallows panics. This is in v1, the stable version.

v2 has this code instead:

    defer func() {
        if r := recover(); r != nil {
            // Try to detect a known reflection error from *this scope*, rather than
            // swallowing all panics that may happen when calling an Action func.
            s := fmt.Sprintf("%v", r)
            if strings.HasPrefix(s, "reflect: ") && strings.Contains(s, "too many input arguments") {
                err = NewExitError(fmt.Sprintf("ERROR unknown Action error: %v.  See %s", r, appActionDeprecationURL), 2)
            } else {
                panic(r)
            }
        }
    }()

Which correctly re-panic() when needed.

Is there any plan to back port this code soon? I don't want to use v2 until it is considered stable, but I don't want to use a broken v1 either :-)

I've made the fix on my side for now. I could submit a pull request if you want but I did not review 100% the changes between v1 and v2 that checks for that reflection error which I never encountered so far to validate that the code in V2 works correctly in v1. Trapping any of MY panics was more important in the short term :-)

[jszwedko]

Hi @mponton,

I'm sorry you ran into that issue and had to spend time tracking it down. This had previously been reported as #451 and fixed in #452 (this fix exists on master and v1) -- is it possible that you were on an older version? v2 actually rips this logic out completely as it no longer needs to use reflection since the previous (deprecated) signature was dropped.

Let me know if I'm missing something!

[mponton]

No worries. I did not mean to sound angry, I definitely appreciate all the work in this library and love it!

I think the issue is that gopkg.in points to an older version and does not track master.

I did a go get gopkg.in/urfave/cli.v1 and the version I get this way does not include the fix and a bunch of other changes. I wanted to make sure I was locked to v1 so that I don't update to v2 automatically when it becomes master.

Thanks!

[jszwedko]

Ah! I see, it has been fixed, but not released. I will tag a release shortly.

[jszwedko]

Tagged and released as v1.18.0 (see release notes for additional changes) and verified that gopkg.in updated its reference to point to it.

Let me know if that addresses this @mponton!

[mponton]

Just did a go get gopkg.in/urfave/cli.v1 and it pulled the fixed version. Thanks a lot for the quick resolution! We never get this level of support for enterprise software :-) Cheers!