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 data race issues in EphemeralGarbageCollector tests #2023

Merged
merged 3 commits into from
Jul 22, 2024
Merged

Fix data race issues in EphemeralGarbageCollector tests #2023

merged 3 commits into from
Jul 22, 2024

Conversation

nadongjun
Copy link
Contributor

  • have read the CONTRIBUTING.md file
  • raised a GitHub issue or discussed it on the projects chat beforehand
  • added unit tests
  • added integration tests
  • updated documentation if needed
  • updated CHANGELOG.md
  • This PR addresses data race issues detected by the -race flag during gotestsum -- -short -coverprofile=coverage.out ./... execution for the EphemeralGarbageCollector tests.

The changes include:

  • Added necessary synchronization with mutex locks to prevent concurrent access issues.
  • Ensured that shared data structures are properly protected during test execution.

Log

  • gotestsum -- -race -coverprofile=coverage.out ./...
=== Failed
=== FAIL: hscontrol/db TestEphemeralGarbageCollectorOrder (6.00s)
==================
WARNING: DATA RACE
Read at 0x00c00057e198 by goroutine 276:
  github.com/juanfont/headscale/hscontrol/db.TestEphemeralGarbageCollectorOrder()
      /Users/dongjunna/headscale/hscontrol/db/node_test.go:620 +0x414
  testing.tRunner()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1742 +0x40

Previous write at 0x00c00057e198 by goroutine 293:
  github.com/juanfont/headscale/hscontrol/db.TestEphemeralGarbageCollectorOrder.func1()
      /Users/dongjunna/headscale/hscontrol/db/node_test.go:605 +0xa4
  github.com/juanfont/headscale/hscontrol/db.(*EphemeralGarbageCollector).Start.gowrap1()
      /Users/dongjunna/headscale/hscontrol/db/node.go:792 +0x44

Goroutine 276 (running) created at:
  testing.(*T).Run()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1742 +0x5e4
  testing.runTests.func1()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:2161 +0x80
  testing.tRunner()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1689 +0x180
  testing.runTests()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:2159 +0x6e0
  testing.(*M).Run()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:2027 +0xb74
  main.main()
      _testmain.go:101 +0x2b4

Goroutine 293 (finished) created at:
  github.com/juanfont/headscale/hscontrol/db.(*EphemeralGarbageCollector).Start()
      /Users/dongjunna/headscale/hscontrol/db/node.go:792 +0x94
  github.com/juanfont/headscale/hscontrol/db.TestEphemeralGarbageCollectorOrder.gowrap1()
      /Users/dongjunna/headscale/hscontrol/db/node_test.go:607 +0x34
==================
==================
WARNING: DATA RACE
Read at 0x00c000010f38 by goroutine 276:
  reflect.Value.Uint()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/reflect/value.go:2753 +0xa80
  github.com/google/go-cmp/cmp.(*state).compareAny()
      /Users/dongjunna/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:277 +0x894
  github.com/google/go-cmp/cmp.(*state).statelessCompare()
      /Users/dongjunna/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:232 +0xbc
  github.com/google/go-cmp/cmp.(*state).compareSlice.func2()
      /Users/dongjunna/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:480 +0xdc
  github.com/google/go-cmp/cmp/internal/diff.Difference()
      /Users/dongjunna/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/internal/diff/diff.go:286 +0x4b8
  github.com/google/go-cmp/cmp.(*state).compareSlice()
      /Users/dongjunna/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:479 +0xad0
  github.com/google/go-cmp/cmp.(*state).compareAny()
      /Users/dongjunna/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:291 +0xf70
  github.com/google/go-cmp/cmp.Diff()
      /Users/dongjunna/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:122 +0x94
  github.com/juanfont/headscale/hscontrol/db.TestEphemeralGarbageCollectorOrder()
      /Users/dongjunna/headscale/hscontrol/db/node_test.go:620 +0x458
  testing.tRunner()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1742 +0x40

Previous write at 0x00c000010f38 by goroutine 293:
  github.com/juanfont/headscale/hscontrol/db.TestEphemeralGarbageCollectorOrder.func1()
      /Users/dongjunna/headscale/hscontrol/db/node_test.go:605 +0x8c
  github.com/juanfont/headscale/hscontrol/db.(*EphemeralGarbageCollector).Start.gowrap1()
      /Users/dongjunna/headscale/hscontrol/db/node.go:792 +0x44

Goroutine 276 (running) created at:
  testing.(*T).Run()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1742 +0x5e4
  testing.runTests.func1()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:2161 +0x80
  testing.tRunner()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1689 +0x180
  testing.runTests()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:2159 +0x6e0
  testing.(*M).Run()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:2027 +0xb74
  main.main()
      _testmain.go:101 +0x2b4

Goroutine 293 (finished) created at:
  github.com/juanfont/headscale/hscontrol/db.(*EphemeralGarbageCollector).Start()
      /Users/dongjunna/headscale/hscontrol/db/node.go:792 +0x94
  github.com/juanfont/headscale/hscontrol/db.TestEphemeralGarbageCollectorOrder.gowrap1()
      /Users/dongjunna/headscale/hscontrol/db/node_test.go:607 +0x34
==================
    testing.go:1398: race detected during execution of test

=== FAIL: hscontrol/db TestEphemeralGarbageCollectorLoads (10.01s)
==================
WARNING: DATA RACE
Read at 0x00c00057e2a0 by goroutine 294:
  github.com/juanfont/headscale/hscontrol/db.TestEphemeralGarbageCollectorLoads()
      /Users/dongjunna/headscale/hscontrol/db/node_test.go:647 +0x48c
  testing.tRunner()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1742 +0x40

Previous write at 0x00c00057e2a0 by goroutine 4873:
  github.com/juanfont/headscale/hscontrol/db.TestEphemeralGarbageCollectorLoads.func1()
      /Users/dongjunna/headscale/hscontrol/db/node_test.go:636 +0x11c
  github.com/juanfont/headscale/hscontrol/db.(*EphemeralGarbageCollector).Start.gowrap1()
      /Users/dongjunna/headscale/hscontrol/db/node.go:792 +0x44

Goroutine 294 (running) created at:
  testing.(*T).Run()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1742 +0x5e4
  testing.runTests.func1()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:2161 +0x80
  testing.tRunner()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:1689 +0x180
  testing.runTests()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:2159 +0x6e0
  testing.(*M).Run()
      /nix/store/05saqcgidraqmn4z82prsp7rbj9hjmwm-go-1.22.5/share/go/src/testing/testing.go:2027 +0xb74
  main.main()
      _testmain.go:101 +0x2b4

Goroutine 4873 (finished) created at:
  github.com/juanfont/headscale/hscontrol/db.(*EphemeralGarbageCollector).Start()
      /Users/dongjunna/headscale/hscontrol/db/node.go:792 +0x94
  github.com/juanfont/headscale/hscontrol/db.TestEphemeralGarbageCollectorLoads.gowrap1()
      /Users/dongjunna/headscale/hscontrol/db/node_test.go:638 +0x34
==================
    testing.go:1398: race detected during execution of test

kradalby
kradalby reviewed Jul 22, 2024
View reviewed changes
hscontrol/db/node_test.go Outdated
got = append(got, ni)
mu.Unlock()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a defer since its a closure.

@kradalby
Copy link
Collaborator

We should probably have a github action that run with the datarace option.

kradalby
kradalby reviewed Jul 22, 2024
View reviewed changes
hscontrol/db/node_test.go
got = append(got, ni)
defer mu.Unlock()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defer must go before the got =

@kradalby kradalby enabled auto-merge (squash) July 22, 2024 15:09
@kradalby kradalby merged commit 4ad3f3c into juanfont:main Jul 22, 2024
107 of 109 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kradalby kradalby kradalby approved these changes

@juanfont juanfont Awaiting requested review from juanfont juanfont is a code owner

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

Successfully merging this pull request may close these issues.
2 participants
@nadongjun @kradalby