3
\$\begingroup\$

Golang does not contain and is not intended to contain a Try-Catch-Finally mechanism but does have a stack unwinding panic and recover mechanism which people have used to simulate exception handling.

As a pure Golang learning exercise I reinvented the try-catch-finally wheel (and then updated it with parts of the more elegant code from the example) which does work correctly as far as I can tell. That this is not a canonical golang pattern is not relevant to this activity.

This code review request is about how to test such a construct thoroughly (but of course any logic flaws in the try-catch-finally is welcome) and in a well structured manner. This test passes and provides full coverage but I am uncertain if I have tested all non-trivial cases or special behaviours relating to language possibilities/behaviour and I would like any stylistic / test structure improvement suggestions that you may wish to provide.

The code to be unit tested:

package goUtil

// PanicException performs a traditional try, catch, finally block.
// You can't rethrow a panic so you have to consider when you really want to catch.
// If you catch a panic then unless you harvest the stack trace then you have lost it.
func PanicException(try func(), catch func(interface{}), finally func()) {
    if finally != nil {
        // Ensure any finally is executed
        defer finally()
    }
    if catch != nil {
        defer func() {
            if r := recover(); r != nil {
                // Execute the catch passing in the panic object
                catch(r)
            }
        }()
    }
    // try and invoke the function call
    try()
}

My unit test code for review:

package goUtil

import (
    "fmt"
    "testing"
)

func TestTryCatchFinallyOperation(t *testing.T) {
    i := 1
    obj := &i
    innerTryCalled := false
    innerTryCompletes := false
    innerCatchCalled := false
    innerCatchCompleted := false
    middleTryCompletes := false
    middleCatchCalled := false
    middleFinallyCalled := false
    outerTryCompletes := false
    outerFinallyCalled := false

    // The inner try panics, the inner catch re-panics but the middle catch swallows the panic
    PanicException(func() {
        // Outer try
        PanicException(func() {
            // Middle try
            PanicException(func() {
                // Inner try; throw panic
                innerTryCalled = true
                panic(obj)
                innerTryCompletes = true
            }, func(e3 interface{}) {
                // Inner catch; re-panic
                innerCatchCalled = true
                FailIfFalse(t, e3 == obj, "The panic object has wrongly changed")
                panic(e3)
                innerCatchCompleted = true
            }, nil)
            middleTryCompletes = true
        }, func(e2 interface{}) {
            // Middle catch; swallow panic
            middleCatchCalled = true
            FailIfFalse(t, e2 == obj, "The panic object has wrongly changed")
        }, func() {
            // Middle finally
            middleFinallyCalled = true
        })
        outerTryCompletes = true
    }, nil,
        func() {
            // Outer fina
            outerFinallyCalled = true
        })

    // Check the execution path
    FailIfFalse(t, innerTryCalled == true, "The inner try was wrongly never called")
    FailIfFalse(t, innerTryCompletes == false, "The inner try should not have completed execution => panic did not unwind stack")
    FailIfFalse(t, innerCatchCalled == true, "The inner catch failed to catch the panic")
    FailIfFalse(t, innerCatchCompleted == false, "The inner catch failed to re-panic")
    FailIfFalse(t, middleTryCompletes == false, "The middle try should not have completed => the inner catch failed to re-panic")
    FailIfFalse(t, middleCatchCalled == true, "The middle catch failed to cath the panic")
    FailIfFalse(t, middleFinallyCalled == true, "The middle finally was not called when there was no panic")
    FailIfFalse(t, outerTryCompletes == true, "The outer try did not complete execution => the middle catch falied to catch the panic")
    FailIfFalse(t, outerFinallyCalled == true, "The outer finally failed to run when there was no panic")

    // No panic should escape the TryCatchFinally stack above which would trigger the test to fail automatically
}


type TestObj interface {
    Log(args ...interface{})
    Fail()
}

// FailIfFalse will invoke t.Fail() if pass is false and generate a log message if failFormat is not ""/nil.
// The failArgs are optional and if present will be args into a t.Logf(failFormat, failArgs...) equivalent call.
// The value of pass is returned.
func FailIfFalse(t TestObj, pass bool, failFormat string, failArgs ...interface{}) bool {
    if !pass {
        if len(failFormat) != 0 {
            var txt string
            if str, _, _, _, ok := GetFileAndLineNo(1); ok {
                txt = "\n" + str + "> "
            }
            if len(failArgs) == 0 {
                txt += failFormat
            } else {
                txt += fmt.Sprintf(failFormat, failArgs...)
            }

            t.Log(txt)
        }
        t.Fail()
    }
    return pass
}

Supporting file and line function:

package goUtil

import (
    "fmt"
    "path/filepath"
    "runtime"
)

// GetFileAndLineNo returns information about the file and line in skip levels up the call tree (0 = location this function is called)
func GetFileAndLineNo(skip int) (text string, pc uintptr, file string, line int, ok bool) {
    pc, file, line, ok = runtime.Caller(skip + 1)
    if ok {
        text = fmt.Sprintf("%s:%d", filepath.Base(file), line)
    }
    return text, pc, file, line, ok
}
\$\endgroup\$
2
\$\begingroup\$

As Elias is strongly urging you, this is not a good idea. Custom ways of doing things fragments codebases and makes library use difficult and frustrating. The possible consequences on the code structure is potentially encouraging an excessive use of lamda-like functionality (Although a really good object like style can mitigate that somewhat) and unreadability due to expression complexity.

That said, to answer your question, I can't see anything you have missed with your unit test coverage. gotest gives you a good code coverage, but that by itself does not mean you have tested all language features.

Golang guarentees that defers will be called at the end of a function irrespective of how the function exits, with a panic or not. Recover will return control flow to you unless there is a major major problem (such as out of memory); it's not clear what would happen then.

Specifically you have successfully covered:

  • 'throwing' a panic
  • 'catching' a panic
  • 'finallying' a panic
  • 'rethrowing' a panic

Note that any code that really panics will have it's stack trace display corrupted by your panic handlers. Any bug reports that show the stack trace will be of reduced value.

\$\endgroup\$
1
\$\begingroup\$

while your approach is really quite an interesting one, I have to say that the basic idea of creating a try-catch-finally mechanism is something I would advise against.

Like you said, go doesn't have a try-catch ATM, and it's unlikely that these constructs will be added any time soon (if ever). The language designers took a different approach to error handling. If you haven't already, I recommend you take a look at this golang blog post on error handling in go. Also make sure to check out posts on the defer, panic, and recover flow and look around for any other posts that might be of interest. There's really a fair bit of useful information in the go blog.

TL;DR, though, I think you're trying to make golang do something in a way that comes natural to you, because you've used try-catch-finally constructs in other languages. The thing is: Go is Go, not one of those other languages. To get the most out of any tool, learn to use it in the way it was intended to be used, don't make it do things other languages do. If you're honest, you must know that this is a bit of hack, and hacks are bad, mkay.

For one, your catch callback relies on the try code to panic. This means that you're likely to over-use panic in your code. You'll have seen a great number of resources on go telling you to avoid using panic as much as possible, your approach not only encourages it: it depends on panic, meaning you're not exactly following best practice.

You're asking here about how to unit-test this code. There are 2 problems with this, however:

  • Have you considered the implications of your approach WRT unit-testing code that "catches" panics? How are you going to unit-test the other code that uses your try-catch stuff? It looks to me like you're making life difficult there
  • Mistakes happen. We've all had tests fail because there's some code that derferences a nil pointer at some point in time, or causes runtime panics for other reasons. If you recover from that runtime panic in your tests, you're likely to miss these critical bugs. Debugging your code will be a pain then.

I guess what I'm saying is: I like the idea, and the remarkable ease with which you managed to mimic the try-catch-finally flow in go is a good example of how expressive the language can be. But to use this in real life would mean your code is harder to maintain, test, and debug. It would make diagnosing problems harder and the apps you write will probably be less robust. For that reason, I wouldn't use your approach.

\$\endgroup\$
  • 1
    \$\begingroup\$ Elias, thank you for taking the time to make this response. It is a good summary of a particular view on error handling. Such a discussion is not what I wanted to start or pursue with this review. Is there any way I can rephrase the review request or a different site that I should use to focus on the original intent: "Does golang have any behaviour / feature that makes the provided unit test be incomplete or misleading"? \$\endgroup\$ – RidiculousRichard Jun 19 '17 at 20:39
  • \$\begingroup\$ @RidiculousRichard: I understood your question, I just got a bit side-tracked. In essence, though, what I tried to say was that you're essentially trying to re-implement a language feature. You can write unit tests that cover 100% of your code paths, but that doesn't take away from the fact that when you use your implementation of try-catch, the using code will be less testable: you're exploiting panic-recover. Any panic therefore will be recovered, and this might hide bugs (eg a runtime panic is recovered in a test that checks for a different error). \$\endgroup\$ – Elias Van Ootegem Jun 21 '17 at 16:54
  • \$\begingroup\$ In this case, you'll end up with passing unit tests, even though the code is broken. Like many people, I wasn't entirely sold on go's error type at first. However, with panic-recover you lose a lot of useful info (that a traditional try-catch also provides!). You can return specific error values, or even error types. A panic is a panic. In some ways, what you did puts me in mind of Brendan Eich's quote on eval: "When all you have is the eval hammer, everything looks like your thumb". Using panic to break out of a function, is like using a wrecking ball to leave your flat: overkill \$\endgroup\$ – Elias Van Ootegem Jun 21 '17 at 16:57
  • 1
    \$\begingroup\$ Thank you for the amplification. I appreciate the effort you have put in \$\endgroup\$ – RidiculousRichard Jun 21 '17 at 22:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.