search

prometheus-community / promql-langserver

PromQL language server
Apache License 2.0
176 stars 18 forks source link

cache: Fix data race in AddDocument #230

Closed prashantv closed 2 years ago

prashantv commented 2 years ago

AddDocument checks if there's an existing document with a matching URI, but it accesses the documents map without locking. Since the documents map is protected by mu, we need to get the lock (ideally RLock).

Update the check to use GetDocuments, since it uses an RLock to read the documents map.

This change also includes a test that intentionally uses concurrent access which triggers the race detector without the fix:

WARNING: DATA RACE
Write at 0x00c00011ee40 by goroutine 8:
  runtime.mapassign_faststr()
      [...]/go1.16.5.darwin.amd64/src/runtime/map_faststr.go:202 +0x0
  github.com/prometheus-community/promql-langserver/langserver/cache.(*DocumentCache).AddDocument()
      [...]/dev/promql-langserver/langserver/cache/cache.go:81 +0x71e
  github.com/prometheus-community/promql-langserver/langserver/cache.TestCacheConcurrent.func1()
      [...]/dev/promql-langserver/langserver/cache/cache_test.go:291 +0xd1

Previous read at 0x00c00011ee40 by goroutine 9:
  runtime.mapaccess2_faststr()
      [...]/versions/go1.16.5.darwin.amd64/src/runtime/map_faststr.go:107 +0x0
  github.com/prometheus-community/promql-langserver/langserver/cache.(*DocumentCache).AddDocument()
      [...]/dev/promql-langserver/langserver/cache/cache.go:49 +0xca
  github.com/prometheus-community/promql-langserver/langserver/cache.TestCacheConcurrent.func1()
      [...]/dev/promql-langserver/langserver/cache/cache_test.go:291 +0xd1
slrtbtfs commented 2 years ago

Good catch, thanks!