Fix broadcast data races

[JohnStarich]

Requirements

• [x] I have added test coverage for new or changed functionality
• [x] 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 https://github.com/launchdarkly/go-server-sdk/issues/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.

[JohnStarich]

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/go@1.21/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0 testing.(*T).Run.func1() $BREW/Cellar/go@1.21/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/go@1.21/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0 testing.(*T).Run.func1() $BREW/Cellar/go@1.21/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/go@1.21/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/go@1.21/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/go@1.21/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0 testing.(*T).Run.func1() $BREW/Cellar/go@1.21/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/go@1.21/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0 testing.(*T).Run.func1() $BREW/Cellar/go@1.21/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/go@1.21/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0 testing.(*T).Run.func1() $BREW/Cellar/go@1.21/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/go@1.21/1.21.7/libexec/src/testing/testing.go:1595 +0x1b0 testing.(*T).Run.func1() $BREW/Cellar/go@1.21/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]

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]

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: https://github.com/launchdarkly/go-server-sdk/pull/152.

Please review when able.

[JohnStarich]

Thanks for taking a look! Reviewed!

[cwaldren-ld]

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!