Anti-pattern: Overwriting return value inside defer

Over the years, I have observed numerous examples of code overriding the return value within a defer statement. This pattern is quite common in my professional experience. In almost every project I have been involved in, I have encountered this exact scenario at some point.

func doSomething() error {
    return errors.New("something went wrong")
}

func doSomethingElse() error {
    return errors.New("something else went wrong")
}

func run() (err error) {
    defer func() {
        err = doSomethingElse()
    }()

    if err = doSomething(); err != nil {
        return err
    }

    return nil
}

Many believe that the f.run() function will return errors.New(“something went wrong”), as the err = doSomethingElse() is considered the "root cause" of the error. However, the actual output of this function is errors.New(“something else went wrong”) and here's why.

The f.run() function returns a named parameter err (Result Parameter) that is overwritten within the defer statement. This approach is intended to stop the program’s execution if the defer statement fails. While this may appear to be a clever solution, it can result in unexpected behavior and introduce bugs into the code.

The first time I encountered this issue was while debugging a pool of workers that were randomly causing invalid memory address or nil pointer dereference. My team and I were relatively new to Go. It was my first year after switching from PHP, and it took almost a week to identify the root cause of the problem.

A few days ago, I came across a similar concept in my ongoing project. I posted a question on LinkedIn to understand how many people are aware of this anti-pattern. This pattern can lead to unexpected behavior and bugs in the code.

The Core Mechanism

In the function signature func run() (err error), the variable err is a result parameter. According to the Go specification:

• It is declared and initialized to its zero value (nil for error types) upon entry to the function.
• It is treated like any other local variable within the function body. You can read from it and assign to it.
• Crucially, if the function executes a return statement without arguments (a "naked" return), the current value of the result parameter is what's returned.

When function returns err, it's explicitly returning the current value of this specific variable. This variable err lives in the function's stack frame and acts as the designated memory location for the return value.

Let's trace the execution of f.run() step-by-step.

func run() (err error) {                                // Step 1
    defer func() {                                              // Step 2
        err = doSomethingElse()
    }()

    if err = doSomething(); err != nil {    // Step 3
        return err                                                  // Step 4
    }

    return nil
}

STEP 1: The run function is called and the result parameter err is declared and initialized within function's stack frame. Its initial value is nil and is part of this frame.

+------------------------------------+
| run() Stack Frame                  |
+------------------------------------+
| err (Result Parameter): nil        | <-- the return slot
+------------------------------------+

STEP 2: The defer statement compiles into a closure. The closure captures a reference to the err variable. This closure is then pushed onto a per-goroutine stack of deferred calls.

STEP 3: The expression err = doSomething() is executed. The error value is assigned to the result parameter err. The value of err in the stack frame is now errors.New("something went wrong").

+------------------------------------+
| run() Stack Frame                  |
+------------------------------------+
| err (Result Parameter): "err_A"    | <-- "something went wrong"
+------------------------------------+

STEP 4: In this step things are getting complicated. The return statement triggers a two-phase process:

• The return expression is evaluated and locks in its current value as the value to be used for the return. errors.New("something went wrong") for now.

• Before the function f.run() officially returns control to f.main(), the runtime executes any deferred calls in LIFO order. Inside the closure, the statement err = doSomethingElse() is run. Because the closure holds a reference to the result parameter err, this assignment overwrites the value in f.run()'s stack frame. The value of the result parameter err is now errors.New("something else went wrong").
  

+------------------------------------+
| run() Stack Frame                  |
|                                    |
|   +----------------------------+   |
|   | defer() closure executing  |   |
|   |                            |   |
|   | err = doSomethingElse()    | --+-\
|   +----------------------------+   | |
|                                    | |
| err (Result Parameter): "err_A"    | | <-- overwrites err
+------------------------------------+ |
                                       |
<--------------------------------------+

When defer completes the value of the result parameter err is overwritten.

+------------------------------------+
| run() Stack Frame                  |
+------------------------------------+
| err (Result Parameter): "err_B"    | <-- "something else went wrong"
+------------------------------------+

The function now completes its return sequence, returning the current value from the err slot. The value returned is "err_B" or "something else went wrong".

The Danger

At this point, you might ask a question: "But I don't see anything wrong here." Perhaps, if you have noticed the danger, my congratulations. Let’s have a beer together.

Let’s take a look at the scenario I faced five years ago and explore the reasons behind the week-long debugging process during my junior year.

A pool of parallel workers processing a huge file containing a list of JSON parameters. The idea was simple: read -> unmarshal into struct -> notify -> return the result in a channel.

The f.notify function was responsible for sending a message to Kafka. The f.process() function itself was executed by the worker.

func process() (result SomeType, err error) {
    defer func {
        err = notify()
    }()

    res, err := readAndUnmarshal()
    if err != nil {
        return
    }

    return
}

func worker(resChan chan SomeOtherType) {
    res, err := process()
    if err != nil {
        resChan <- SomeOtherType{Err: err}
    }
    resChan <- SomeOtherType{Result: res}
}

We were not provided with the exact content of the JSON file we were reading. Instead, we were given a small chunk, which were used to construct our result struct. The JSON keys may contain invalid values. For example, instead of an object, there might be a number. This will result in the unmarshaling failing, and an error will be returned.

res, err := readAndUnmarshal()
if err != nil {
    return   // this return well be triggered
}

This return statement will initiate the deferred closure’s execution. The f.notify() will be executed, overriding the error returned by the f.readAndUnmarshal() function with nil. The return value of the f.process() function when f.readAndUnmarshal() fails will be nil, nil. The worker will then push a nil value to the channel SomeOtherType{Result: nil}.

In the runtime, when accessing the field within the result of f.worker(), the program will cause a panic.

// panic: runtime error: invalid memory address or nil pointer dereference
value := msg.Result.SomeField

The defer block completely broke the error path of the program. The required object was not initialized but was still returned. This error required a significant amount of time and effort to identify and rectify. Even experienced developers overlooked this error. I was able to reproduce this error by accidentally passing an empty JSON to be parsed.

Honestly, I have no idea what the workers initially wrote in such a way. I also don't know the intention behind this design decision. The project was handed over in the ready-to-production state. Even unit tests were written in a way that f.notify() guaranteed to fail, and this case was never tested.

Unnamed return value scenario

Let's also discuss the case when a function returns a local variable as an error. Here, the f.run() function will return errors.New(“something went wrong”).

func doSomething() error {
    return errors.New("something went wrong")
}

func doSomethingElse() error {
    return errors.New("something else went wrong")
}

func run() error {
    var err error // error is a local variable this time

    defer func() {
        err = doSomethingElse()
    }()

    if err = doSomething(); err != nil {
        return err
    }

    return nil
}

In this scenario, when f.run() is called and A stack frame is created. It contains a slot for the local variable err and a separate, anonymous slot for the return value.

+------------------------------------+
| run() Stack Frame                  |
+------------------------------------+
| __return_value_0 (hidden): nil     | <-- return slot
| err (local variable):     nil      | <-- local variable
+------------------------------------+

After executing err = doSomething() the local err variable is updated.

+------------------------------------+
| run() Stack Frame                  |
+------------------------------------+
| __return_value_0 (hidden): nil     |
| err (local variable):     "err_A"  | <-- "something went wrong"
+------------------------------------+

At this point return is triggeted. This is the most important difference. The copy the value of the local err variable into the hidden return slot." This action happens before the defer is executed.

+------------------------------------+
| run() Stack Frame                  |
+------------------------------------+
| __return_value_0 (hidden): "err_A" | <-- RETURN VALUE IS NOW LOCKED IN
| err (local variable):      "err_A" |
+------------------------------------+

The deferred closure runs. It has a reference to the local err variable, not the hidden return slot.

+------------------------------------+
| run() Stack Frame                  |
+------------------------------------+
| __return_value_0 (hidden): "err_A" | <-- Unchanged
| err (local variable):      "err_B" | <-- Overwritten
+------------------------------------+

The function returns the value from its hidden return slot. The value returned is "err_A".

The return mechanism in Go is more complex because defer and named return values create a multi-step process that gives the programmer explicit control over the function's exit logic. This explicit two-phase process is a distinguishing feature of Go. In other languages, the process is more opaque. When a return is hit in a try block, the language runtime knows a finally block must run, but the interaction with the return value itself is often less defined and considered an anti-pattern to exploit.

Comments

[Dan Jones]

Here's how I'd write your process function:

func process() (result SomeType, err error) {
    defer func {
        err = errors.Join(err, notify())
    }()

    result, err = readAndUnmarshal()
    return
}

We don't need the if err != nil check, because we're returning the named values.

And, by using errors.Join, we avoid the problem of simple overwriting the variable.

errors.Join(nil, nil) returns nil. errors.Join(anErr, nil) returns anErr directly (without wrapping it). errors.Join(anErr, anotherErr) returns the two wrapped together, so you get both error messages, and can still use errors.Is and errors.As to inspect the individual errors.

Used correctly, this isn't an anti-pattern.

[Harutyun Mardirossian]

Yes, that’s exactly what we did. We removed all unnecessary code and joined the errors. I’m still surprised that the initial team hasn’t noticed that the code was faulty.