Race condition when using mutex and reflect

user_732886 0
Viewed 578

The Go race detector is reporting a race condition when using a mutex and reflect on the struct being locked, example code is below. Even though both the reflect and access to the struct member is protected by locking the mutex the race detector still reports a race condition.

How can I solve the contention?

Code:

// main file
package main

import (
    "fmt"
    "reflect"
    "sync"
)

type TestType struct {
    counter uint64
    lock sync.Mutex
}

func NewTestType() *TestType {
    t := &TestType{
        counter: 0,
        lock:    sync.Mutex{},
    }

    go func() {
        t.lock.Lock()
        defer t.lock.Unlock()
        t.counter++
    }()

    return t
}

func ItShouldNotRace() string {
    t := NewTestType()

    t.lock.Lock()
    defer t.lock.Unlock()

    val := reflect.ValueOf(t)
    iface := val.Interface()
    return fmt.Sprintf("%v", iface)
}

// test file
package main

import (
    "testing"
)

func TestItShouldNotRace(t *testing.T) {
    if ItShouldNotRace() == "impossible" {
        t.Fail()
    }
}

Race detector output:

==================
WARNING: DATA RACE
Read at 0x00c000120078 by goroutine 7:
  reflect.typedmemmove()
      /usr/local/opt/go/libexec/src/runtime/mbarrier.go:177 +0x0
  reflect.packEface()
      /usr/local/opt/go/libexec/src/reflect/value.go:120 +0x12f
  reflect.valueInterface()
      /usr/local/opt/go/libexec/src/reflect/value.go:1045 +0x1cd
  reflect.Value.Interface()
      /usr/local/opt/go/libexec/src/reflect/value.go:1015 +0x3aa4
  fmt.(*pp).printValue()
      /usr/local/opt/go/libexec/src/fmt/print.go:726 +0x3aa5
  fmt.(*pp).printValue()
      /usr/local/opt/go/libexec/src/fmt/print.go:880 +0x25fc
  fmt.(*pp).printArg()
      /usr/local/opt/go/libexec/src/fmt/print.go:716 +0x26b
  fmt.(*pp).doPrintf()
      /usr/local/opt/go/libexec/src/fmt/print.go:1030 +0x326
  fmt.Sprintf()
      /usr/local/opt/go/libexec/src/fmt/print.go:219 +0x73
  go_issue.ItShouldNotRace()
      /Users/dedalus/Downloads/go_issue/main.go:37 +0x1a4
  go_issue.TestItShouldNotRace()
      /Users/dedalus/Downloads/go_issue/main_test.go:8 +0x2f
  testing.tRunner()
      /usr/local/opt/go/libexec/src/testing/testing.go:1127 +0x202

Previous write at 0x00c000120078 by goroutine 8:
  sync/atomic.CompareAndSwapInt32()
      /usr/local/opt/go/libexec/src/runtime/race_amd64.s:293 +0xb
  sync.(*Mutex).lockSlow()
      /usr/local/opt/go/libexec/src/sync/mutex.go:129 +0x14b
  sync.(*Mutex).Lock()
      /usr/local/opt/go/libexec/src/sync/mutex.go:81 +0x84
  go_issue.NewTestType.func1()
      /Users/dedalus/Downloads/go_issue/main.go:21 +0x47

Goroutine 7 (running) created at:
  testing.(*T).Run()
      /usr/local/opt/go/libexec/src/testing/testing.go:1178 +0x796
  testing.runTests.func1()
      /usr/local/opt/go/libexec/src/testing/testing.go:1449 +0xa6
  testing.tRunner()
      /usr/local/opt/go/libexec/src/testing/testing.go:1127 +0x202
  testing.runTests()
      /usr/local/opt/go/libexec/src/testing/testing.go:1447 +0x5aa
  testing.(*M).Run()
      /usr/local/opt/go/libexec/src/testing/testing.go:1357 +0x4eb
  main.main()
      _testmain.go:43 +0x236

Goroutine 8 (running) created at:
  go_issue.NewTestType()
      /Users/dedalus/Downloads/go_issue/main.go:20 +0x7a
  go_issue.ItShouldNotRace()
      /Users/dedalus/Downloads/go_issue/main.go:30 +0x54
  go_issue.TestItShouldNotRace()
      /Users/dedalus/Downloads/go_issue/main_test.go:8 +0x2f
  testing.tRunner()
      /usr/local/opt/go/libexec/src/testing/testing.go:1127 +0x202
==================
--- FAIL: TestItShouldNotRace (0.00s)
    testing.go:1042: race detected during execution of test
go mutex
2 Answers

This data race can be solved by making lock a pointer:

type TestType struct {
    counter uint64
    lock *sync.Mutex
}

From sync.Mutex docs:

// A Mutex must not be copied after first use.

Using a value type for lock field leads to copying this mutex.

user_43784000

The problem here is not the reflect value at all, you can replicate the race with a simpler example:

type TestType struct {
    sync.Mutex
}

func main() {
    t := &TestType{}

    go func() {
        t.Lock()
        defer t.Unlock()
    }()

    t.Lock()
    defer t.Unlock()

    fmt.Printf("%v\n", t)
}

The issue is that the fmt.Printf call is going to walk through the values provided in order to format the output, and in that process it must read the sync.Mutex value itself. This means that the race is in the reading of mutex value (a mutex should not be "read", as it's not a value that can be copied. The fact that fmt does read and print a private mutex value at all is debatably a bug, but not something that can be really be changed at this point)

If this is a value that you intend to pass to fmt frequently for output, then I would suggest adding String, GoString and/or Format methods to create a string value without reading the mutex itself.

Simply adding fmt.Stringer to the example above avoids the race condition.

func (t *TestType) String() string {
    return "TestType{}"
}

This is also convenient if you need to lock the value in order to read other fields as well. If we add back in your counter field, we need to serialize that access (note you need to remove the external lock call to prevent a deadlock here):

func (t *TestType) String() string {
    t.Lock()
    defer t.Unlock()
    return fmt.Sprintf("TestType{counter:%d}", t.counter)
}

The other way to avoid this is to use a pointer for a mutex, however that prevents you from using the zero value which tends to be preferred, and you still need to ensure that you externally lock the value so the other fields can be safely read. Since you already have a constructor, that may not be an issue, but it is something to consider

type TestType struct {
    *sync.Mutex
}

func NewTestType() *TestType {
    return &TestType{
        &sync.Mutex{},
    }
}
Jim_328800
Related