locations.go sourceInfoRegistry cache is not concurrent

[apodznoev]

We found the locations.go a useful utility to access locations of HTTP options, in particular: MethodHTTPRule.

However, it is not concurrent and might crash like:

fatal error: concurrent map writes

goroutine 5017 [running]:
github.com/googleapis/api-linter/locations.sourceInfoRegistryType.sourceInfo(0x14000172e70, 0x140052a1ea0)
        github.com/googleapis/[email protected]/locations/locations.go:86 +0x144
github.com/googleapis/api-linter/locations.pathLocation({0x1022ba130, 0x140052af6c0}, {0x14001072b18, 0x2, 0x14001072b58?})
        github.com/googleapis/[email protected]/locations/locations.go:37 +0xfc
github.com/googleapis/api-linter/locations.FieldResourceReference(0x140052af6c0)
        github.com/googleapis/[email protected]/locations/field_locations.go:35 +0x68

With goroutine:

goroutine 5024 [runnable]:
github.com/googleapis/api-linter/locations.sourceInfoRegistryType.sourceInfo(0x14000172e70, 0x140039f5180)
        github.com/googleapis/[email protected]/locations/locations.go:81 +0x104
github.com/googleapis/api-linter/locations.pathLocation({0x1022ba090, 0x140037cf8b0}, {0x14004b87af8, 0x2, 0x2?})
        github.com/googleapis/[email protected]/locations/locations.go:37 +0xfc
github.com/googleapis/api-linter/locations.MethodOption(...)
        github.com/googleapis/[email protected]/locations/method_locations.go:54
github.com/googleapis/api-linter/locations.MethodHTTPRule(0x140037cf8b0)
        github.com/googleapis/[email protected]/locations/method_locations.go:37 +0x68

or:

goroutine 4853 [runnable]:
github.com/jhump/protoreflect/desc/internal.asMapKey(...)
        github.com/jhump/[email protected]/desc/internal/source_info.go:52
github.com/jhump/protoreflect/desc/internal.SourceInfoMap.Get(0xc002394ae0, {0xc0010bd810, 0x4, 0x5bc2d70?})
        github.com/jhump/[email protected]/desc/internal/source_info.go:14 +0x3f
github.com/jhump/protoreflect/desc.(*FieldDescriptor).GetSourceInfo(0x6c7ada0?)
        github.com/jhump/[email protected]/desc/descriptor.go:963 +0x2d
github.com/googleapis/api-linter/locations.pathLocation({0x5bb15d0, 0xc0012561c0}, {0xc00692eb70, 0x2, 0x5bba138?})
        github.com/googleapis/[email protected]/locations/locations.go:33 +0x43
github.com/googleapis/api-linter/locations.FieldResourceReference(0xc0012561c0)
        github.com/googleapis/[email protected]/locations/field_locations.go:35 +0x66

[noahdietz]

Yeah you are totally right. Cool that you are using it for what I'm guessing are non ApiLinter use cases? Or if you are, using goroutines in a linter rule is interesting and I'd like to hear more.

Anyways, I'll work on a change to add locking around the map access.

I don't particularly want to use a sync.Map, so I'll just refactor the internal types to structs with map and sync.Mutex and go from there.

[apodznoev]

Thanks for working on it @noahdietz, really appreciated!

In our use case, we have a CLI in Go that contains a linter combining AIP rules and some self-written ones specific to our domain.

When writing our rules we need to report problematic locations in protos to the end user, and the path from the SourceCodeInfo_Location is what we are using to find line numbers in the source code.

Since we already have AIP linter as a dependency, we found the locations.go a convenient way to detect the Location,
but since our rules are running as goroutines, the problem above appears.

[noahdietz]

Is the reason for running the rules in "parallel" via goroutines simply for speed of execution?

[noahdietz]

Fix is out for review. Hopefully I can get it released today for ya

[noahdietz]

Fixed in https://github.com/googleapis/api-linter/releases/tag/v1.67.3!

[apodznoev]

Is the reason for running the rules in "parallel" via goroutines simply for speed of execution

👍
correct, they are independent, and we see no reason to run them sequentially.