Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: [pkg/proxy] fix data races #746

Merged
merged 1 commit into from
Mar 25, 2025
Merged

fix: [pkg/proxy] fix data races #746

merged 1 commit into from
Mar 25, 2025
+57 −36

Conversation

crandles
Copy link
Contributor

@crandles crandles commented Mar 22, 2025 •
edited
Loading

Fixing data races in pkg/proxy. (In conjunction with #743, this should bring us to zero races)

  • concurrent adds to a sync.waitgroup
    • Fix: replaced with sync.Cond + int
Race Condition 
WARNING: DATA RACE
Write at 0x00c000868028 by goroutine 10389:
  runtime.racewrite()
      <autogenerated>:1 +0x10
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.(*progressiveCollapseForwarder).WaitAllComplete()
      /Users/c/code/oss/trickster/pkg/proxy/engines/progressive_collapse_forwarder.go:147 +0x3c
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.TestPCFWaits.func2()
      /Users/c/code/oss/trickster/pkg/proxy/engines/progressive_collapse_forwarder_test.go:164 +0x44

Previous read at 0x00c000868028 by goroutine 10388:
  runtime.raceread()
      <autogenerated>:1 +0x10
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.(*progressiveCollapseForwarder).AddClient()
      /Users/c/code/oss/trickster/pkg/proxy/engines/progressive_collapse_forwarder.go:96 +0x4c
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.TestPCFWaits.gowrap1()
      /Users/c/code/oss/trickster/pkg/proxy/engines/progressive_collapse_forwarder_test.go:161 +0x60

Goroutine 10389 (running) created at:
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.TestPCFWaits()
      /Users/c/code/oss/trickster/pkg/proxy/engines/progressive_collapse_forwarder_test.go:163 +0x420
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1792 +0x180
  testing.(*T).Run.gowrap1()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1851 +0x40

Goroutine 10388 (running) created at:
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.TestPCFWaits()
      /Users/c/code/oss/trickster/pkg/proxy/engines/progressive_collapse_forwarder_test.go:161 +0x37c
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1792 +0x180
  testing.(*T).Run.gowrap1()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1851 +0x40
  • concurrent read and write to byte slice
    • fix: guard byte slice with mutex
Race Condition 
WARNING: DATA RACE
Read at 0x00c0007224b0 by goroutine 9354:
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.(*progressiveCollapseForwarder).IndexRead()
      /Users/c/code/oss/trickster/pkg/proxy/engines/progressive_collapse_forwarder.go:201 +0x130
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.(*progressiveCollapseForwarder).AddClient()
      /Users/c/code/oss/trickster/pkg/proxy/engines/progressive_collapse_forwarder.go:104 +0x8c
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.handlePCF()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache.go:340 +0x7c8
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.handleCacheKeyMiss()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache.go:268 +0x168
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.fetchViaObjectProxyCache()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache.go:436 +0x970
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.ObjectProxyCacheRequest()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache.go:470 +0x58
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.testFetchOPC()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache_test.go:1124 +0x138
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.TestObjectProxyCacheRequestWithPCFChunks()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache_chunk_test.go:497 +0x3ac
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1792 +0x180
  testing.(*T).Run.gowrap1()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1851 +0x40

Previous write at 0x00c0007224b0 by goroutine 9362:
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.(*progressiveCollapseForwarder).Write()
      /Users/c/code/oss/trickster/pkg/proxy/engines/progressive_collapse_forwarder.go:170 +0x1bc
  io.(*multiWriter).Write()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/io/multi.go:85 +0xa8
  io.copyBuffer()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/io/io.go:431 +0x258
  io.Copy()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/io/io.go:388 +0x460
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.handlePCF.func1()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache.go:335 +0x94

Goroutine 9354 (running) created at:
  testing.(*T).Run()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1851 +0x684
  testing.runTests.func1()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:2279 +0x7c
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1792 +0x180
  testing.runTests()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:2277 +0x77c
  testing.(*M).Run()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:2142 +0xb68
  main.main()
      _testmain.go:389 +0x110

Goroutine 9362 (running) created at:
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.handlePCF()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache.go:329 +0x79c
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.handleCacheKeyMiss()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache.go:268 +0x168
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.fetchViaObjectProxyCache()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache.go:436 +0x970
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.ObjectProxyCacheRequest()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache.go:470 +0x58
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.testFetchOPC()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache_test.go:1124 +0x138
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.TestObjectProxyCacheRequestWithPCFChunks()
      /Users/c/code/oss/trickster/pkg/proxy/engines/objectproxycache_chunk_test.go:497 +0x3ac
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1792 +0x180
  testing.(*T).Run.gowrap1()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1851 +0x40
  • multiple go routines modifying Resources struct (time range query field)
    • Fix: added mutex to Resources struct and lock/unlock before accessing fields
Race Condition 
WARNING: DATA RACE
Write at 0x00c0005c5088 by goroutine 1077:
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.DeltaProxyCacheRequest()
      /Users/c/code/oss/trickster/pkg/proxy/engines/deltaproxycache.go:74 +0x434
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.(*TestClient).QueryRangeHandler()
      /Users/c/code/oss/trickster/pkg/proxy/engines/client_test.go:378 +0x1ec
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.TestDeltaProxyCacheRequestPartialHitChunks()
      /Users/c/code/oss/trickster/pkg/proxy/engines/deltaproxycache_chunk_test.go:388 +0xfb8
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1792 +0x180
  testing.(*T).Run.gowrap1()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1851 +0x40

Previous read at 0x00c0005c5088 by goroutine 1161:
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.WriteCache()
      /Users/c/code/oss/trickster/pkg/proxy/engines/cache.go:408 +0x450
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.DeltaProxyCacheRequest.func2()
      /Users/c/code/oss/trickster/pkg/proxy/engines/deltaproxycache.go:419 +0x3cc

Goroutine 1077 (running) created at:
  testing.(*T).Run()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1851 +0x684
  testing.runTests.func1()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:2279 +0x7c
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1792 +0x180
  testing.runTests()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:2277 +0x77c
  testing.(*M).Run()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:2142 +0xb68
  main.main()
      _testmain.go:389 +0x110

Goroutine 1161 (finished) created at:
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.DeltaProxyCacheRequest()
      /Users/c/code/oss/trickster/pkg/proxy/engines/deltaproxycache.go:405 +0x4188
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.(*TestClient).QueryRangeHandler()
      /Users/c/code/oss/trickster/pkg/proxy/engines/client_test.go:378 +0x1ec
  github.com/trickstercache/trickster/v2/pkg/proxy/engines.TestDeltaProxyCacheRequestPartialHitChunks()
      /Users/c/code/oss/trickster/pkg/proxy/engines/deltaproxycache_chunk_test.go:349 +0x658
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1792 +0x180
  testing.(*T).Run.gowrap1()
      /opt/homebrew/Cellar/go/1.24.1/libexec/src/testing/testing.go:1851 +0x40

@coveralls
Copy link

coveralls commented Mar 22, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 14048967772

Details

  • 40 of 40 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 86.366%
Totals Coverage Status
Change from base Build 14047151783: 0.03%
Covered Lines: 16508
Relevant Lines: 19114
💛 - Coveralls

@crandles
fix: [pkg/proxy] fix data races
bdfb853 
Signed-off-by: Chris Randles <randles.chris@gmail.com>
@crandles crandles marked this pull request as ready for review March 25, 2025 12:22
@crandles crandles merged commit 7cc2b53 into trickstercache:main Mar 25, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jranson jranson jranson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@crandles @coveralls @jranson