Fun and headaches with Closures in Go

Learn from my mistake and don’t re-use variables

Sustainability is becoming more important by the minute, as countries across the globe come to terms with the ongoing climate crisis.

So I decided to recycle as much as I can — including variables in my Go code — and got a nasty bug and a headache to boot.

Good idea, poor execution

If you’re an engineer, you have to appreciate that idiom. At times, it’s the only description that fits. Tight schedules, demands flying in from all angles, and ever-increasing complexity: it all leads to messy code.

In my case, it was the factory closure that got me. While building an auth mechanism for gRPC services, I stumbled on an article describing the factory pattern and thought “oh my, this is a good fit for an authz override”.

Start with a good idea

I would have a global function that validates JWT tokens, and would let each gRPC service add its own validation via a hook. And since most of Authz is RBAC or ABAC, I want to re-use validation code. So I made a factory that spits out validation functions for a basic RBAC system. Each gRPC server then implements the auth hook via an interface, and calls the factory with the intended role.

Line 23 introduces the bug. But why?

AuthUnaryInterceptor is a factory method. It returns gRPC interceptors that validate IdP tokens on every call. And because I always over-engineer some tiny aspect, it receives an optional authFunc so you can add custom auth logic (and the idea of a factory makes more sense).

The Good idea evolves

Each gRPC server can also implement an interface called ServiceAuthFuncOverride to return one of these authFunc types itself. So I thought, hey, I’ll call the override function if it exists, and I’ll put it in the authFunc variable. Then, I’ll call it. This way, if the global was passed to the factory method, it will be called. If the server defines an override, it will be saved in the authFunc variable, replacing the global, and the override will get called instead.

if overrideSrv, ok := info.Server.(ServiceAuthFuncOverride); ok {
  // THIS IS WHERE I FAIL:
  authFunc = overrideSrv.AuthFuncOverride(newCtx, info.FullMethod)
}
if authFunc != nil {
  newCtx, errF = authFunc(newCtx)
}

Here comes the fun part

So why does the by-now-infamous line 23 fail? Remember how closures work. They tie a unnamed function to its immediate scope; in this case, the localauthFunc from the main function is still local in the scope of the unnamed function. Since it’s passed by reference, the first time it gets overridden, it stays that way for all future calls.

This type of bug manifests in unpredictable ways. It is a logic error which depends on the order of calls, so it may seem random and reproducing it will be elusive. Say you have two gRPC servers: ServerA uses the global auth, while ServerB has an override. You call a method on ServerA, then on ServerB. Stuff works as expected, but authFunc is overridden globally (in the scope of all of our interceptors at least). You then call ServerA again, and it fails, because our buggy Auth will apply the override function from ServerB. If you write a ServerC with its own override, it will work because the buggy Auth will override authFuncagain.

Prevent it, don’t just fix it

A quick fix is easy: don’t re-use authFunc . Just use a new variable and everything works as expected. To prevent this from happening, you just need to keep in mind how scope works with closures.

However, as I write this, I see that it’s the style of coding that is at fault. I knew all of this going in, but it still took me a while to figure out what was happening. What got me into trouble is my compact style, where I re-use variables and rely on more logic than I should.

Observe:

// re-using s is compact, but complicated
func F(s string, bool override) {
  if override {
    s = get_string()
  }
  if s != "" {
    save_string(s)
  }
}
// It's better to use another var, and be explicit
func F(s string, bool override) {
 var str string
 if override {
   str = get_string()
 } else {
  str = s
 }
 if str != "" {
   save_string(str)
 }
}

In the second example, the intention is clearer, and it would have avoided my scoping bug as well.

Do you have any suggestions on how to improve this?