Fix broadcast data races #151

JohnStarich · 2024-05-16T23:38:13Z

Requirements

I have added test coverage for new or changed functionality
I have followed the repository's pull request submission guidelines
I have validated my changes against all supported platform versions
- What does "platform" mean here? Go version? OS version? Happy to help test, just not sure what I'm testing!

Related issues

I hit data races when running tests in parallel, which effectively obscures other data races my application may have. It looks like others did too, judging by #102.
Fixes #102

Describe the solution you've provided

I've addressed 2 data races with improved locking:

HasListeners() had a data race due to any mutation on b.subscribers
Close() had a data race where closing a send channel triggers a panic in Broadcast()

I also saw an easy opportunity to use more fine-grained locks with an RWMutex, although I'm happy to back that out if you would prefer.

Describe alternatives you've considered

I also considered using an atomic data type for subscribers, but I figured that change would be less of a surgical fix. It also may be more difficult to mutate the slice, since a compare-and-swap can fail and would need a loop (it's not as simple as atomic.Add).

Another idea which may be simpler is using a channel to manage shutdown. Typically I'd use a context.Context here to manage cancellation. That'd also prevent Broadcast()ing on a full send channel from blocking Close().

Additional context

Add any other context about the pull request here.

Address data races between: * HasListeners and any other method using Lock() * Broadcast and Close Switch to a RWMutex for finer-grained locking.

JohnStarich · 2024-05-16T23:50:03Z

For further details, here's some snapshots of the data races I can produce with the first commit 83abad0:

HasListeners() and Close() (any mutation of b.subscribers would do)

==================
WARNING: DATA RACE
Read at 0x00c00006a4a0 by goroutine 34:
  github.com/launchdarkly/go-server-sdk/v7/internal.(*Broadcaster[go.shape.string]).HasListeners()
      $PWD/internal/broadcasters.go:70 +0x30
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func4()
      $PWD/internal/broadcasters_test.go:97 +0x18
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func6()
      $PWD/internal/broadcasters_test.go:107 +0x7c

Previous write at 0x00c00006a4a0 by goroutine 33:
  github.com/launchdarkly/go-server-sdk/v7/internal.(*Broadcaster[go.shape.string]).Close()
      $PWD/internal/broadcasters.go:92 +0x104
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func3()
      $PWD/internal/broadcasters_test.go:96 +0x34
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func6()
      $PWD/internal/broadcasters_test.go:107 +0x7c

Goroutine 34 (running) created at:
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace()
      $PWD/internal/broadcasters_test.go:105 +0x39c
  testing.tRunner()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0
  testing.(*T).Run.func1()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1648 +0x40

Goroutine 33 (finished) created at:
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace()
      $PWD/internal/broadcasters_test.go:105 +0x39c
  testing.tRunner()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0
  testing.(*T).Run.func1()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1648 +0x40
==================
--- FAIL: TestBroadcasterDataRace (0.00s)
    testing.go:1465: race detected during execution of test
FAIL
FAIL	github.com/launchdarkly/go-server-sdk/v7/internal	1.231s
FAIL

Broadcast() and Close()

==================
WARNING: DATA RACE
Write at 0x00c000096070 by goroutine 31:
  runtime.recvDirect()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/runtime/chan.go:348 +0x7c
  github.com/launchdarkly/go-server-sdk/v7/internal.(*Broadcaster[go.shape.string]).Close()
      $PWD/internal/broadcasters.go:90 +0xe0
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func3()
      $PWD/internal/broadcasters_test.go:96 +0x34
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func6()
      $PWD/internal/broadcasters_test.go:107 +0x7c

Previous read at 0x00c000096070 by goroutine 29:
  runtime.chansend1()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/runtime/chan.go:146 +0x2c
  github.com/launchdarkly/go-server-sdk/v7/internal.(*Broadcaster[go.shape.string]).Broadcast()
      $PWD/internal/broadcasters.go:80 +0x17c
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func2()
      $PWD/internal/broadcasters_test.go:95 +0x40
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func6()
      $PWD/internal/broadcasters_test.go:107 +0x7c

Goroutine 31 (running) created at:
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace()
      $PWD/internal/broadcasters_test.go:105 +0x39c
  testing.tRunner()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0
  testing.(*T).Run.func1()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1648 +0x40

Goroutine 29 (finished) created at:
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace()
      $PWD/internal/broadcasters_test.go:105 +0x39c
  testing.tRunner()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0
  testing.(*T).Run.func1()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1648 +0x40
==================
==================
WARNING: DATA RACE
Write at 0x00c000216100 by goroutine 31:
  github.com/launchdarkly/go-server-sdk/v7/internal.(*Broadcaster[go.shape.string]).Close()
      $PWD/internal/broadcasters.go:92 +0x104
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func3()
      $PWD/internal/broadcasters_test.go:96 +0x34
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func6()
      $PWD/internal/broadcasters_test.go:107 +0x7c

Previous read at 0x00c000216100 by goroutine 33:
  github.com/launchdarkly/go-server-sdk/v7/internal.(*Broadcaster[go.shape.string]).HasListeners()
      $PWD/internal/broadcasters.go:70 +0x30
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func4()
      $PWD/internal/broadcasters_test.go:97 +0x18
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace.func6()
      $PWD/internal/broadcasters_test.go:107 +0x7c

Goroutine 31 (running) created at:
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace()
      $PWD/internal/broadcasters_test.go:105 +0x39c
  testing.tRunner()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0
  testing.(*T).Run.func1()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1648 +0x40

Goroutine 33 (finished) created at:
  github.com/launchdarkly/go-server-sdk/v7/internal.TestBroadcasterDataRace()
      $PWD/internal/broadcasters_test.go:105 +0x39c
  testing.tRunner()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0
  testing.(*T).Run.func1()
      $BREW/Cellar/[email protected]/1.21.7/libexec/src/testing/testing.go:1648 +0x40
==================
--- FAIL: TestBroadcasterDataRace (0.00s)
    testing.go:1465: race detected during execution of test
FAIL
FAIL	github.com/launchdarkly/go-server-sdk/v7/internal	0.877s
FAIL

cwaldren-ld · 2024-05-17T18:34:14Z

Hi @JohnStarich , thank you for the contribution, this looks good. I will take a deeper look at this next week.

Filed internally as 244791.

cwaldren-ld · 2024-05-28T21:14:41Z

Hi @JohnStarich , I've merged this PR into a feature branch prior to merging to the main branch.

I made an additional set of changes here: #152.

Please review when able.

JohnStarich · 2024-05-29T00:01:02Z

Thanks for taking a look! Reviewed!

cwaldren-ld · 2024-06-04T18:33:28Z

This has been released in https://github.com/launchdarkly/go-server-sdk/releases/tag/v7.4.1. Please file another issue if you have any other problems. Thanks!

JohnStarich added 2 commits May 16, 2024 18:27

Add failing test for data races in broadcasters.go

83abad0

Fix broadcast data races

77bf4db

Address data races between: * HasListeners and any other method using Lock() * Broadcast and Close Switch to a RWMutex for finer-grained locking.

JohnStarich requested a review from a team as a code owner May 16, 2024 23:38

cwaldren-ld changed the base branch from v7 to cw/sc-244791/broadcast-data-race May 28, 2024 21:08

cwaldren-ld merged commit 9b387be into launchdarkly:cw/sc-244791/broadcast-data-race May 28, 2024
12 checks passed

dlamotte mentioned this pull request May 29, 2024

Data race in internal/broadcasters.go #102

Closed

JohnStarich deleted the bugfix/fix-broadcast-data-race branch June 5, 2024 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix broadcast data races #151

Fix broadcast data races #151

JohnStarich commented May 16, 2024 •

edited

JohnStarich commented May 16, 2024

cwaldren-ld commented May 17, 2024

cwaldren-ld commented May 28, 2024

JohnStarich commented May 29, 2024

cwaldren-ld commented Jun 4, 2024 •

edited

Fix broadcast data races #151

Fix broadcast data races #151

Conversation

JohnStarich commented May 16, 2024 • edited

JohnStarich commented May 16, 2024

cwaldren-ld commented May 17, 2024

cwaldren-ld commented May 28, 2024

JohnStarich commented May 29, 2024

cwaldren-ld commented Jun 4, 2024 • edited

JohnStarich commented May 16, 2024 •

edited

cwaldren-ld commented Jun 4, 2024 •

edited