Hacker News Reader

Comment by cratermoon | Hacker News Reader

Relevant to the Race condition on error from (*Rows).Close. The Tour of Go notes, "The deferred call’s arguments are evaluated immediately, but the function call is not executed until the surrounding function returns." Problems with deferring Close() on writable files have been known for some time, and the recommendation I have is not to defer just the close, but defer a function or block that does the close and handles any error return. In this case, though, the dangling buffers in the driver introduce a problem that goes beyond just handling an error on close.

View on HN

One pattern I happily use is to capture the error into a named return error, using a multi-error if the named error is already non-nil.

    func fetchFoo(ctx context.Context) (_ Foo, mErr error) {
        conn := pool.Acquire(ctx)
        defer errs.Capture(&mErr, conn.Close, "close conn for fetch foo")
    }

I have a couple of useful variants:

        defer errs.CaptureLog(logger, conn.Close, "close conn")
        defer errs.CaptureT(t, conn.Close, "close conn")

To avoid the function overhead, you can also defer the Close() and explicitly call Close() at the end:

    defer rows.Close()
    /* do things */
    err = rows.Close()
    if err != nil {
    }

1. If the explicit Close() is called first, the error from the deferred Close() is ignored.

2. If the explicit Close() is never called because of a panic, the deferred Close() will kick in and cleanup the resources.

Is that actually faster? You're saving the (small) overhead of an extra function, but you're adding a new function call. Certainly in the context of an SQL query (which will take milliseconds at the least) I wouldn't expect a few nanoseconds to matter.

That's true. However I also like to not mess around with any deferred functions that have to capture an error variable by name (named returns) or by pointer so that it can modify the error on the way out. Calling rows.Close() after you're done with rows is a simple 4-line addition that doesn't require rewriting the already existing defer rows.Close(), which lowers the cognitive load in my opinion.

Like you said, the performance gains (if any) are probably not going to matter when a database call is involved.

I prefer not to use defer at all in these cases. It’s too confusing. Just put the stuff that you would do between the defer and the end of the function in another function. It’s almost always clearer.

That's less safe, though: in a request handler that catches panics, you'll leak connections but stay running if something snags in the row collection logic you write, which isn't what you want.

I also prefer not to use request handlers that recover from panics. In my codebases, a panic means fail now and fail hard. :-)

I think if there’s a panic somewhere that you’re not explicitly expecting you can say goodbye to any notion of safety you might have had.

HTTP request handlers are self-contained (or rather, should be), and more analogous to a process on a system than a function call. Bringing down the entire application is akin to bringing down the entire OS because one process segfaults.

Most panics are small stupid things localized to a specific piece of code, like nil dereference or slice out of bounds, and not indicative of some global process state being messed up.

My HTTP handlers tend to share state between invocations, such as caches and database drivers. If the cache or database driver panics then I don’t trust the cache or db to continue to be correct, so the only sensible thing is to shut the server down.

In my deployments, a server is just one in a pool of servers, so losing one is not a problem. The whole system is designed such that any server can die at any time and the overall service will continue to be available.

If you have a different architecture and design philosophy to me then I can see why you would want to recover in your handlers. It just doesn’t suit me at all.

I figured you'd have something like that. Works well enough if you have a pool of servers, but it's basically a non-starter for anything that you want to be self-hosted for example, or other scenarios where things are small enough in scale that "pool of servers" isn't really worth it.

Cases where it panics and leaves an incorrect state seem very rare; panics due to an incorrect state are slightly less rare, but still fairly rare.

Reply on news.ycombinator.com