current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

Consider the following function:

func Listen(username string) (net.Listener, error) {
    path := filepath.Join(socketsDir, username)

    oldUmask := syscall.Umask(^0600)
    defer syscall.Umask(oldUmask)

    listener, err := net.Listen("unixpacket", path)
    if err != nil {
        return nil, err
    }

    passwd, err := passwd.Getpwnam(username)
    if err != nil {
        goto chownFailed
    }

    err = os.Chown(path, int(passwd.Uid), os.Getgid())
    if err != nil {
        goto chownFailed
    }

    return listener, err

chownFailed:
    listener.Close()
    return nil, err
}

I think this way of error handling is ugly. Is there a way to get rid of the goto without duplicating the call to listener.Close?

share|improve this question

2 Answers 2

up vote 4 down vote accepted

You could use defer along with named return values.

E.g.:

listener, err := net.Listen("unixpacket", path)
if err != nil {
    return nil, err
}
defer func() {
    if err != nil {
        listener.Close()
    }
}()

// rest of function that just does "return nil, err" as needed

A runnable playground version of this: https://play.golang.org/p/lKnGyzbp_3

Note, normally you'll see things like this in Go:

f, err := os.Open("somefile")
if err != nil {
    return err
}
defer f.Close()

That is easier and works for the common case where the thing (a file here) is being used just inside the function itself and needs to always be closed before returning to the caller. In the question however, the listening socket needs to be closed only if there is a error after it was opened. If there is no error it needs to be left open and returned to the caller. That's why the defer'ed function checks err (which it can do as long as it's a named returned value; you can even change named return values in a defer'ed function).

share|improve this answer
    
You don't need an closure there though. Just defer listener.Close() suffices. –  Ainar-G Mar 10 at 8:02
    
@Ainar-G err, no? –  elyse Mar 10 at 8:19
    
@райтфолд What do you mean? The error from net.Listen has already been checked before, so you don't need to check err inside the deferred function. –  Ainar-G Mar 10 at 8:29
    
@Ainar-G no you can't (a simple defer is what I had originally answered until I read the question more closely). If an error doesn't occur an open listener needs to be returned. Always defering close is for things you are just using in the function itself. –  Dave C Mar 10 at 16:13
up vote 2 down vote

That's the standard C cleanup block, which is extremely widely used, you can see tons of examples just browsing the Linux kernel code. In more structured programming languages you have two (other) options:

  • You can use a try..finally block. Simply return nil or the proper pair and finally will trigger either way to do cleanup. This is the equivalent of C#'s using, which I consider to be very graceful and semantic code. In Windows using SEH, such a block is pretty much free in terms of performance, only actually throwing and stack unwinding takes time.
  • You can also define a lambda function at the beginning called cleanup() (or just fail()) and call it when appropriate. I consider this to be less elegant because if you forget to call it you'll have a leak.

Alternatively redesign your code to use RIAA. Go is a scoping language right? Like C++? Simply wrap your objects in a class and rely on destructors for cleanup.

share|improve this answer
    
your "answer" irrelevant to this question since the question is all about Go. Go doesn't have try…finally and it doesn't have destructors (it's garbage collected, it does have finalizers but they are inappropriate here). –  Dave C Mar 10 at 16:27

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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