ReverseGeocode thread-safety

[mologie]

Currently, concurrent calls to ReverseGeocode will have undefined results due to iterator reuse between concurrent calls. The devil is in the detail, specifically a comment on s2.ContainsPointQuery mentions that:

This type is not safe for concurrent use.

The Go race detector does not find this issue by default, because tests are not parallelized. I add a separate commit to parallelize the tests and thus enable detection of the issue via go test -count=1 -race -run=TestReverseGeocode_Countries:

$ go test -count=1 -race -run=TestReverseGeocode_Countries
==================
WARNING: DATA RACE
Write at 0x00c000431628 by goroutine 28:
  github.com/golang/geo/s2.(*ShapeIndexIterator).seek()
      /Users/okuckertz/go/pkg/mod/github.com/golang/[email protected]/s2/shapeindex.go:315 +0xf3
  github.com/golang/geo/s2.(*ShapeIndexIterator).LocatePoint()
      /Users/okuckertz/go/pkg/mod/github.com/golang/[email protected]/s2/shapeindex.go:331 +0x70
  github.com/golang/geo/s2.(*ContainsPointQuery).visitContainingShapes()
      /Users/okuckertz/go/pkg/mod/github.com/golang/[email protected]/s2/contains_point_query.go:164 +0x89
  github.com/golang/geo/s2.(*ContainsPointQuery).ContainingShapes()
      /Users/okuckertz/go/pkg/mod/github.com/golang/[email protected]/s2/contains_point_query.go:181 +0xa4
  github.com/sams96/rgeo.(*Rgeo).ReverseGeocode()
      /Users/okuckertz/Devel/AV/rgeo/rgeo.go:160 +0xf0
  github.com/sams96/rgeo.TestReverseGeocode_Countries.func1()
      /Users/okuckertz/Devel/AV/rgeo/rgeo_test.go:284 +0xad
  testing.tRunner()
      /usr/local/Cellar/go/1.22.4/libexec/src/testing/testing.go:1689 +0x21e
  testing.(*T).Run.gowrap1()
      /usr/local/Cellar/go/1.22.4/libexec/src/testing/testing.go:1742 +0x44
[...]

Solution

The issue can be avoided by simply constructing a new query for every lookup.

Performance evaluation

I found that this change does not impact test runtime, because ShapeIndex internally optimizes to build the index only once for the first query in a thread-safe manner. Unchanged timing can be verified via time go test -count=1 -parallel=1.

However, due to changed timing of New and the first ReverseGeocode call, this merge request should not land in a patch but rather a minor release with appropriate release notes that mention this detail. A new Build method is added to expose ShapeIndex.Build if the user wants to avoid indexing overhead with the first lookup.

Best,
Oliver

[codecov]

Codecov Report

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

Project coverage is 97.88%. Comparing base (9db03ae) to head (79ae70f).

Files	Patch %	Lines
rgeo.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
- Coverage   98.58%   97.88%   -0.70%     
==========================================
  Files           2        2              
  Lines         141      142       +1     
==========================================
  Hits          139      139              
- Misses          1        2       +1     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mologie]

Oh, I made the bot angry. Please lkm if you want a test for Build() to be covered. This would need to verify timings or internal properties of the index set to be non-nonsensical, so a bit more effort for something that is essentially just pass-through anyway.