Data race within Go function with named return values?

user_7414116 0
Viewed 260

Question

Is there a non-obvious data race within the following code between qux() writing to the err at the same time when a new error baz is being returned? Is it possible that Go internal handling of the named return values treats explicit returning of a new variable as a de facto assignment to the named return value and thus allowing a data race here?

func foo(baz time.Duration) (bar *Bar, err error) {
    done := make(chan struct{})
    
    go func() {
        bar, err = qux()
        close(done)
    }()

    select {
    case <-done:
        return
    case <-time.After(baz):
        return nil, errors.New("baz")
    }
}

This does not make any sense for me, as I would expect that result of errors.New() in the above code should be allocated to a different address than the pre-initialized err. However, I have reasons to suspect that this is exactly what happened to me.

Background

One of our services is executing a bit less than a half of a million HTTP requests per second over multiple pods in a cluster. Averagely once per 500 million or so requests we got an irrecoverable fault panic happening during checking of a wrapped error here in the standard library. It happens usually when a non-nil pointer points to a faulty memory address, which suggests concurrent writes.

unexpected fault address 0x0
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x80 addr=0x0 pc=0x40ebfa]

goroutine 7431988 [running]:
runtime.throw(0x1bcf57c, 0x5)
    /usr/local/go/src/runtime/panic.go:1116 +0x72 fp=0xc0026e8d38 sp=0xc0026e8d08 pc=0x43bef2
runtime.sigpanic()
    /usr/local/go/src/runtime/signal_unix.go:727 +0x405 fp=0xc0026e8d68 sp=0xc0026e8d38 pc=0x452805
runtime.getitab(0x195cae0, 0xe93824548b483024, 0x1, 0x7f7fc8037af8)
    /usr/local/go/src/runtime/iface.go:39 +0x3a fp=0xc0026e8db8 sp=0xc0026e8d68 pc=0x40ebfa
runtime.assertI2I2(0x195cae0, 0xdcd388, 0x0, 0x7f7fc8037af8, 0xc018808a20, 0xc012bbaa01)
    /usr/local/go/src/runtime/iface.go:472 +0x6a fp=0xc0026e8de8 sp=0xc0026e8db8 pc=0x41004a
errors.Is(0x1dd3f20, 0xc0120b6780, 0x1dd2a60, 0xc00003c1c0, 0xc019510c60)
    /usr/local/go/src/runtime/wrap.go:49 +0xd8 fp=0xc0026e8e50 sp=0xc0026e8de8 pc=0xaacbf8

The code we run looks more or less like this:

func (c *client) Get(ctx context.Context, query Query) (Results, error) {
    b, err := c.fetchAndLearn(ctx, query)
    if errors.Is(err, context.Canceled) { // Panic happens on this line
        // ...
    }

    // ...
}

func (c *client) fetchAndLearn(ctx context.Context, query Query) (response *http.Response, err error) {
    done := make(chan struct{})

    go func() {
        response, err = c.fetch(ctx, query)
        close(done)
    }()

    select {
    case <-done:
        return
    case <-clock.After(c.getTimeout(query)):
        return nil, xerrors.New("timeout exceeded")
    }
}

After fruitless debugging I wondered if Go was doing something unintuitive under the hood due to the fact that named return values were used here. Having this faint intuition I fixed our issue with irrecoverable panics simply by removing named return values and declaring separate err variables.

func (c *client) fetchAndLearn(ctx context.Context, query Query) (*http.Response, error) {
    done := make(chan struct{})
    var response *http.Response
    var err error

    go func() {
        response, err = c.fetch(ctx, query)
        close(done)
    }()

    select {
    case <-done:
        if err != nil {
            return nil, xerrors.Errorf(": %w", err)
        }
        return response, nil
    case <-clock.After(c.getTimeout(query)):
        return nil, xerrors.New("timeout exceeded during request duration learning")
    }
}

But it still does not make that much sense for me and I would really appreciate if someone with a good understanding of Go inner workings either confirm or deny my suspicion.

go
1 Answers

Answer

Yes, explicit return in a named return function is written to preinitialized named results as per documentation.

Background

If you run following code you will notice that even though in foo() the preinitialized val is never manipulated directly after initial assignment it changes contents after val2 is returned.

package main

import (
    "fmt"
    "time"
)

func main() {
    val := foo()
    fmt.Printf("Main: %+v\n", val.Bar) // explicit
    time.Sleep(2 * time.Second)
}

type Foo struct {
    Bar string
}

func foo() (val *Foo) {
    val = &Foo{Bar: "named"}
    go func() {
        time.Sleep(time.Second)
        fmt.Printf("Goroutine: %+v\n", val.Bar) // explicit
    }()

    val2 := &Foo{Bar: "explicit"}

    return val2
}

This happens because in Go a zero-valued return parameters are initialized on each function start. Then upon return any returned values are assigned to the preinitialized ones. In our example upon return val2 is assigned to val, which then is returned by the function. This opens the possibility of a data race, because even if you are assigning value to a val2 it will eventually be assigned to the val too.

My only excuse is that as of the time of writing this the official effective Go guide is slightly unintuitive as it says that named results are initialized and tied to an unadorned return which falsely presupposes that an explicit return is not tied. Credit goes to helpful guys behind go101.

user_74141160
Related