Data race problem with mutex protected maps

[Rory Campbell-Lange]

I'm trying to work out why I have a data race problem (on go 1.15 and 1.16).

*SearchResults.Set is called from several goroutines. I am trying to avoid concurrent access to its two maps using a mutex, but this isn't working.

108 type SearchResults struct {
109     Boxes map[string][]Box
110     seen  map[string]bool
111     sync.Mutex
112 }
113
114 // Set adds a new box to the search results map
115 func (s *SearchResults) Set(b Box) {
116
117     s.Lock()
118     defer s.Unlock()
119
120     if _, ok := s.seen[b.BoxID]; ok { // race problem
121         return
122     }
123
124     if model := b.GetModel(); model != "" {
125         s.Boxes[model] = append(s.Boxes[model], b) // race problem
126         s.seen[b.BoxID] = true // race problem
127     }
128 }

The data race report from go run -race <program> shows 

==================
WARNING: DATA RACE
Write at 0x00c000012e70 by goroutine 16:
  runtime.mapassign_faststr()
      /home/rory/bin/go/src/runtime/map_faststr.go:202 +0x0
  main.(*SearchResults).Set()
      /home/rory/src/go-cex/cexer.go:126 +0x345
...
Previous read at 0x00c000012e70 by goroutine 8:
  runtime.mapaccess2_faststr()
      /home/rory/bin/go/src/runtime/map_faststr.go:107 +0x0
  main.(*SearchResults).Set()
      /home/rory/src/go-cex/cexer.go:120 +0xf6
...
==================
WARNING: DATA RACE
Read at 0x00c000012ea0 by goroutine 8:
  runtime.mapaccess1_faststr()
      /home/rory/bin/go/src/runtime/map_faststr.go:12 +0x0
  main.(*SearchResults).Set()
      /home/rory/src/go-cex/cexer.go:125 +0x195
...
Previous write at 0x00c000012ea0 by goroutine 16:
  runtime.mapassign_faststr()
      /home/rory/bin/go/src/runtime/map_faststr.go:202 +0x0
  main.(*SearchResults).Set()
      /home/rory/src/go-cex/cexer.go:125 +0x2a7
...

Advice gratefully received.

Rory

[Dan Kortschak]

It looks like the Box type is being mutated and read in more than one
goroutine; you're protecting Set, but is the b value being used
elsewhen?

Dan

[Rodolfo]

Try to use sync.RWMutex instead, you have more control when to read or write.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/78204779089a2167f27992bad6894eaad9bd3575.camel%40kortschak.io.

[Kurtis Rader]

You're going to need to provide more detail. Specifically, a minimal reproducer. There is no obvious explanation for the race given what you've already shown us so I suspect you've omitted an important fact.

--

You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAPQX7QR1Evk7BzKAxwq3GKH%3DgiNb4YVs_ap7Q%2BUVC_OByoty2w%40mail.gmail.com.

--

Kurtis Rader

Caretaker of the exceptional canines Junior and Hank

[Ian Lance Taylor]

On Sat, Jun 26, 2021 at 3:27 PM Rory Campbell-Lange <ror...@gmail.com> wrote:
>

Looks like you have multiple SearchResults values that refer to the same map.

Ian

[Dan Kortschak]

On Sat, 2021-06-26 at 20:44 -0700, Ian Lance Taylor wrote:
> Looks like you have multiple SearchResults values that refer to the
> same map.

Shouldn't that result in a panic, even without -race?

Dan

[Brian Candler]

> Shouldn't that result in a panic, even without -race?

It's not *guaranteed* to panic when you make concurrent accesses to the same map (hence the point of the race checker).  And having two structs refer to the same map is no different to having two variables refer to the same map.  

For the OP: one way you could see this problem is if you ever copy SearchResults by value:

https://play.golang.org/p/LqwXQQzQ4VN

Note the "go vet" warning here.  Have you tried running "go vet" on your code?

[Brian Candler]

One way to be robust against this particular problem is to carry a pointer to a mutex, rather than embedding a mutex.

https://play.golang.org/p/UPY7EBKSYl5

[Rory Campbell-Lange]

On 26/06/21, Ian Lance Taylor (ia...@golang.org) wrote:
> On Sat, Jun 26, 2021 at 3:27 PM Rory Campbell-Lange <ror...@gmail.com> wrote:
> >
> > I'm trying to work out why I have a data race problem (on go 1.15 and 1.16).
> >
> > *SearchResults.Set is called from several goroutines. I am trying to avoid concurrent access to its two maps using a mutex, but this isn't working.
>

> Looks like you have multiple SearchResults values that refer to the same map.

Thanks for the insightful comment -- I was passing SearchResults to the goroutine by value instead of by reference.

By the way, as a golang newbie, it isn't clear to me when it is advisable to
use a channel for locking as opposed to a mutex. Do people tend to use the former?

Thanks again,
Rory

[Brian Candler]

On Sunday, 27 June 2021 at 10:32:22 UTC+1 ro...@campbell-lange.net wrote:

By the way, as a golang newbie, it isn't clear to me when it is advisable to
use a channel for locking as opposed to a mutex. Do people tend to use the former?

There is an absolutely excellent video on this topic by Rob Pike here:

https://www.youtube.com/watch?v=5zXAHh5tJqQ

Once you've seen these patterns, you likely won't want to go back to mutexes.

For example: in your case, I'd create a one-element buffered channel containing the data.  Any function which wants to access that data can pop the value off the channel, use and/or modify it, then push it back into the channel when done.

However if the *only* issue is around concurrent access to a single map, then you could just use sync.Map.

[Rory Campbell-Lange]

On 27/06/21, Brian Candler (b.ca...@pobox.com) wrote:
> > Shouldn't that result in a panic, even without -race?
>
> It's not *guaranteed* to panic when you make concurrent accesses to the
> same map (hence the point of the race checker). And having two structs
> refer to the same map is no different to having two variables refer to the
> same map.
>
> For the OP: one way you could see this problem is if you ever copy
> SearchResults by value:
> https://play.golang.org/p/LqwXQQzQ4VN
>
> Note the "go vet" warning here. Have you tried running "go vet" on your
> code?

Thanks for the very useful example.

I have completely overlooked the use of go vet, which has found a bunch of
"copies lock" and "passes lock by value" errors.

Rory

[Rory Campbell-Lange]

On 27/06/21, Brian Candler (b.ca...@pobox.com) wrote:

> then you could just use sync.Map <https://golang.org/pkg/sync/#Map>.

I considered sync.Map but since I have two maps I thought struct-level
locking might make more sense.

Thank you for the pointers and the excellent video (actually by Bryan C.
Mills, it seems).

errgroup.WithContext looks seriously helpful. While I was aware of the
ability of a channel to act as a semphore the concepts of sharing
through channels comes across very well, particularly sharing resources
and data, concepts which open up a lot of possibilities.

The video is eye-opening but will take a while to digest.

Thanks again
Rory

[Brian Candler]

On Sunday, 27 June 2021 at 19:33:40 UTC+1 ro...@campbell-lange.net wrote:

Thank you for the pointers and the excellent video (actually by Bryan C.
Mills, it seems).

Argh!  My apologies to Bryan.  I copied the link directly from my local bookmarks/notes file, and I had misattributed it there.

It took me several views, and lots of pausing, to fully digest it too.