fix(locations): make source info access concurrent safe #1433
Conversation
|
|
||
| // findLocation returns the Location for a given path. | ||
| func (si sourceInfo) findLocation(path []int32) *dpb.SourceCodeInfo_Location { | ||
| func (si *sourceInfo) findLocation(path []int32) *dpb.SourceCodeInfo_Location { | ||
| si.infoMu.Lock() |
There was a problem hiding this comment.
Do we need to lock on the read of this map? It seems like we will only write the contents of the info map once per key, and I assume we only try to access it after writing, so there shouldn't be a need to lock on read
I could only see a lock being needed if the call to findLocation can happen while another call to sourceInfo() is currently executing and writing the value for that location
Not that it matters too much to have an extra lock call 🤷♀️
There was a problem hiding this comment.
I think tooling such as the test -race flag might complain without the read lock, but I guess you can remove it to see if that happens.
There was a problem hiding this comment.
I could only see a lock being needed if the call to findLocation can happen while another call to sourceInfo() is currently executing and writing the value for that location
Not that it matters too much to have an extra lock call 🤷♀️
Yeah that's what I'm being cautious about - i'd rather just synchronize all access. It's not performant enough of an application to really care about the extra lock call and I'm choosing to go one step short of using sync.Map (which is not strongly typed) :)
There was a problem hiding this comment.
Thanks @slevenick for the point/question though
🤖 I have created a release *beep* *boop* --- ## [1.67.3](https://togithub.com/googleapis/api-linter/compare/v1.67.2...v1.67.3) (2024-09-23) ### Bug Fixes * **AIP-132:** refine List request response regex ([#1420](https://togithub.com/googleapis/api-linter/issues/1420)) ([5cc4d27](https://togithub.com/googleapis/api-linter/commit/5cc4d279c9cfc80545a9d2447b4fe13a8032b2aa)) * **AIP-136:** ignore revision methods ([#1432](https://togithub.com/googleapis/api-linter/issues/1432)) ([a6ba5f3](https://togithub.com/googleapis/api-linter/commit/a6ba5f32458cefc42b662019d97199d0a8e86551)) * **AIP-162:** handle LRO in response name rules ([#1431](https://togithub.com/googleapis/api-linter/issues/1431)) ([8bca1dd](https://togithub.com/googleapis/api-linter/commit/8bca1dd68ccf00c39a06da9862ac8c599029297e)) * **internal/utils:** refine Get, Create, Update, Delete request regex ([#1422](https://togithub.com/googleapis/api-linter/issues/1422)) ([487328c](https://togithub.com/googleapis/api-linter/commit/487328ca8708521562be2921d3c4f2aabaf8a5ae)) * **locations:** make source info access concurrent safe ([#1433](https://togithub.com/googleapis/api-linter/issues/1433)) ([223aa5b](https://togithub.com/googleapis/api-linter/commit/223aa5bb6cf4f24193ad6c6037e1b88160474f2e)) ### Documentation * **AIP-132:** fix incorrect field for AIP-217 ([#1423](https://togithub.com/googleapis/api-linter/issues/1423)) ([6a52a68](https://togithub.com/googleapis/api-linter/commit/6a52a6845bf8f240a4d9f9a305a26609a2699c17)) * **AIP-134:** change mandated to recommended ([#1426](https://togithub.com/googleapis/api-linter/issues/1426)) ([338b6a9](https://togithub.com/googleapis/api-linter/commit/338b6a95906b61ba5a83805bce92919dd53725dc)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Refactors internal source code info registry for the
locationspackage to leverage asynx.Mutexin protecting themaps containing the descriptor/path to SourceCodeInfo pairs. Uses the "mutex hat" pattern for naming/commenting.Adds a simple concurrency test to run with
-race. Note: Running this same test onmainproduced the data race warning.Executes unit tests in CI with the
-raceflag.Updates #1430