open-telemetry / opamp-go

OpAMP protocol implementation in Go
Apache License 2.0
146 stars 71 forks source link

Limit to single inflight package syncing operation #289

Closed tpaschalis closed 2 months ago

tpaschalis commented 4 months ago

This PR revives work done in previous PRs (#118, #120) to make sure that only a single package syncing operation is ever in flight and also adds a test.

The previous PRs did not account for needing to also protect initStatuses and SetPackageStatuses, so that's why the Lock and Unlock statements are not just paired in doSync. If you think the intent would be clearer using a sync.WaitGroup, let me know.

The new test makes sure that the mutex correctly protects the local storage; if we comment out the calls to Lock/Unlock and run the test with the -race flag we can see the race condition taking place

``` # Without using the mutex we can see the race condition of messages sent in parallel $ go test -run=TestPackageUpdatesInParallel -v -race -count=1 === RUN TestPackageUpdatesInParallel ================== WARNING: DATA RACE Write at 0x00c00003cdb0 by goroutine 10: github.com/open-telemetry/opamp-go/client/internal.(*InMemPackagesStore).SetLastReportedStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/inmempackagestore.go:95 +0x34 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).reportStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:335 +0x78 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).syncPackage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:199 +0x4dc github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).doSync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:132 +0x3b0 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync.gowrap1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x4c Previous read at 0x00c00003cdb0 by goroutine 8: github.com/open-telemetry/opamp-go/client/internal.(*InMemPackagesStore).LastReportedStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/inmempackagestore.go:91 +0x30 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).initStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:88 +0x64 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:59 +0x78 github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:185 +0x68 github.com/open-telemetry/opamp-go/client/types.CallbacksStruct.OnMessage() /Users/tpaschalis/GitRepos/opamp-go/client/types/callbacks.go:161 +0x84 github.com/open-telemetry/opamp-go/client/types.(*CallbacksStruct).OnMessage() :1 +0x20 github.com/open-telemetry/opamp-go/client/internal.(*receivedProcessor).ProcessReceivedMessage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/receivedprocessor.go:160 +0xe94 github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func2() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:197 +0x4d0 Goroutine 10 (running) created at: github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x15c github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:185 +0x68 github.com/open-telemetry/opamp-go/client/types.CallbacksStruct.OnMessage() /Users/tpaschalis/GitRepos/opamp-go/client/types/callbacks.go:161 +0x84 github.com/open-telemetry/opamp-go/client/types.(*CallbacksStruct).OnMessage() :1 +0x20 github.com/open-telemetry/opamp-go/client/internal.(*receivedProcessor).ProcessReceivedMessage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/receivedprocessor.go:160 +0xe94 github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func3() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:216 +0x4d0 Goroutine 8 (finished) created at: github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:196 +0x4b0 testing.tRunner() /opt/homebrew/Cellar/go/1.22.2/libexec/src/testing/testing.go:1689 +0x180 testing.(*T).Run.gowrap1() /opt/homebrew/Cellar/go/1.22.2/libexec/src/testing/testing.go:1742 +0x40 ================== ================== WARNING: DATA RACE Write at 0x00c0000999e0 by goroutine 11: runtime.mapaccess2_faststr() /opt/homebrew/Cellar/go/1.22.2/libexec/src/runtime/map_faststr.go:108 +0x42c github.com/open-telemetry/opamp-go/client/internal.(*InMemPackagesStore).CreatePackage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/inmempackagestore.go:50 +0x74 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).syncPackage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:203 +0x528 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).doSync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:132 +0x3b0 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync.gowrap1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x4c Previous read at 0x00c0000999e0 by goroutine 10: runtime.mapdelete() /opt/homebrew/Cellar/go/1.22.2/libexec/src/runtime/map.go:696 +0x43c github.com/open-telemetry/opamp-go/client/internal.(*InMemPackagesStore).Packages() /Users/tpaschalis/GitRepos/opamp-go/client/internal/inmempackagestore.go:36 +0x64 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).deleteUnneededLocalPackages() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:303 +0x58 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).doSync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:125 +0x1b4 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync.gowrap1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x4c Goroutine 11 (running) created at: github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x15c github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:185 +0x68 github.com/open-telemetry/opamp-go/client/types.CallbacksStruct.OnMessage() /Users/tpaschalis/GitRepos/opamp-go/client/types/callbacks.go:161 +0x84 github.com/open-telemetry/opamp-go/client/types.(*CallbacksStruct).OnMessage() :1 +0x20 github.com/open-telemetry/opamp-go/client/internal.(*receivedProcessor).ProcessReceivedMessage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/receivedprocessor.go:160 +0xe94 github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func2() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:197 +0x4d0 Goroutine 10 (running) created at: github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x15c github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:185 +0x68 github.com/open-telemetry/opamp-go/client/types.CallbacksStruct.OnMessage() /Users/tpaschalis/GitRepos/opamp-go/client/types/callbacks.go:161 +0x84 github.com/open-telemetry/opamp-go/client/types.(*CallbacksStruct).OnMessage() :1 +0x20 github.com/open-telemetry/opamp-go/client/internal.(*receivedProcessor).ProcessReceivedMessage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/receivedprocessor.go:160 +0xe94 github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func3() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:216 +0x4d0 ================== ================== WARNING: DATA RACE Write at 0x00c00003cdb0 by goroutine 11: github.com/open-telemetry/opamp-go/client/internal.(*InMemPackagesStore).SetLastReportedStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/inmempackagestore.go:95 +0x34 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).reportStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:335 +0x78 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).syncPackage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:229 +0x780 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).doSync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:132 +0x3b0 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync.gowrap1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x4c Previous write at 0x00c00003cdb0 by goroutine 10: github.com/open-telemetry/opamp-go/client/internal.(*InMemPackagesStore).SetLastReportedStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/inmempackagestore.go:95 +0x34 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).reportStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:335 +0x78 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).syncPackage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:229 +0x780 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).doSync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:132 +0x3b0 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync.gowrap1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x4c Goroutine 11 (running) created at: github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x15c github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:185 +0x68 github.com/open-telemetry/opamp-go/client/types.CallbacksStruct.OnMessage() /Users/tpaschalis/GitRepos/opamp-go/client/types/callbacks.go:161 +0x84 github.com/open-telemetry/opamp-go/client/types.(*CallbacksStruct).OnMessage() :1 +0x20 github.com/open-telemetry/opamp-go/client/internal.(*receivedProcessor).ProcessReceivedMessage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/receivedprocessor.go:160 +0xe94 github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func2() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:197 +0x4d0 Goroutine 10 (running) created at: github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x15c github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:185 +0x68 github.com/open-telemetry/opamp-go/client/types.CallbacksStruct.OnMessage() /Users/tpaschalis/GitRepos/opamp-go/client/types/callbacks.go:161 +0x84 github.com/open-telemetry/opamp-go/client/types.(*CallbacksStruct).OnMessage() :1 +0x20 github.com/open-telemetry/opamp-go/client/internal.(*receivedProcessor).ProcessReceivedMessage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/receivedprocessor.go:160 +0xe94 github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func3() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:216 +0x4d0 ================== ================== WARNING: DATA RACE Write at 0x00c00003cdb0 by goroutine 10: github.com/open-telemetry/opamp-go/client/internal.(*InMemPackagesStore).SetLastReportedStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/inmempackagestore.go:95 +0x34 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).reportStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:335 +0x78 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).doSync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:151 +0x69c github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync.gowrap1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x4c Previous write at 0x00c00003cdb0 by goroutine 11: github.com/open-telemetry/opamp-go/client/internal.(*InMemPackagesStore).SetLastReportedStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/inmempackagestore.go:95 +0x34 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).reportStatuses() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:335 +0x78 github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).doSync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:151 +0x69c github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync.gowrap1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x4c Goroutine 10 (running) created at: github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x15c github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:185 +0x68 github.com/open-telemetry/opamp-go/client/types.CallbacksStruct.OnMessage() /Users/tpaschalis/GitRepos/opamp-go/client/types/callbacks.go:161 +0x84 github.com/open-telemetry/opamp-go/client/types.(*CallbacksStruct).OnMessage() :1 +0x20 github.com/open-telemetry/opamp-go/client/internal.(*receivedProcessor).ProcessReceivedMessage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/receivedprocessor.go:160 +0xe94 github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func3() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:216 +0x4d0 Goroutine 11 (running) created at: github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync() /Users/tpaschalis/GitRepos/opamp-go/client/internal/packagessyncer.go:69 +0x15c github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func1() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:185 +0x68 github.com/open-telemetry/opamp-go/client/types.CallbacksStruct.OnMessage() /Users/tpaschalis/GitRepos/opamp-go/client/types/callbacks.go:161 +0x84 github.com/open-telemetry/opamp-go/client/types.(*CallbacksStruct).OnMessage() :1 +0x20 github.com/open-telemetry/opamp-go/client/internal.(*receivedProcessor).ProcessReceivedMessage() /Users/tpaschalis/GitRepos/opamp-go/client/internal/receivedprocessor.go:160 +0xe94 github.com/open-telemetry/opamp-go/client/internal.TestPackageUpdatesInParallel.func2() /Users/tpaschalis/GitRepos/opamp-go/client/internal/httpsender_test.go:197 +0x4d0 ================== testing.go:1398: race detected during execution of test --- FAIL: TestPackageUpdatesInParallel (0.10s) FAIL exit status 1 FAIL github.com/open-telemetry/opamp-go/client/internal 0.286s ```

Fixes #84

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.89%. Comparing base (ed38d5f) to head (d016331). Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
client/internal/packagessyncer.go 80.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #289 +/- ## ========================================== + Coverage 76.32% 76.89% +0.56% ========================================== Files 25 25 Lines 1681 1692 +11 ========================================== + Hits 1283 1301 +18 + Misses 291 285 -6 + Partials 107 106 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tpaschalis commented 4 months ago

I've rebased main and fixed the conflict from a recent PR, let me know what you think! 🙌

tigrannajaryan commented 2 months ago

Thanks for your patience @tpaschalis