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.

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

WIP: fix remaining data races #10341

Closed
wants to merge 12 commits into from
Closed

Commits on Jun 2, 2021

  1. serf: isolated updateTags logic in one package

    And remove unnecessary functions. This is done in preparation for
    properly locking when updating tags.
    dnephin committed Jun 2, 2021
    Configuration menu
    Copy the full SHA
    51380d1 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    9a4d9d3 View commit details
    Browse the repository at this point in the history
  3. consul: fix data race in leader CA tests

    Some global variables are patched to shorter values in these tests. But the goroutines that read
    them can outlive the test because nothing waited for them to exit.
    
    This commit adds a Wait() method to the routine manager, so that tests can wait for the goroutines
    to exit. This prevents the data race because the 'reset to original value' can happen
    after all other goroutines have stopped.
    dnephin committed Jun 2, 2021
    Configuration menu
    Copy the full SHA
    18f7339 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    0d3d7b6 View commit details
    Browse the repository at this point in the history

Commits on Jun 8, 2021

  1. agent: fix two data race in agent tests

    The LogOutput io.Writer used by TestAgent must allow concurrent reads and writes, and a
    bytes.Buffer does not allow this. The bytes.Buffer must be wrapped with a lock to make this safe.
    dnephin committed Jun 8, 2021
    Configuration menu
    Copy the full SHA
    d036287 View commit details
    Browse the repository at this point in the history
  2. agent: fix a data race in DNS tests

    The dnsConfig pulled from the atomic.Value is a pointer, so modifying it in place
    creates a data race. Use the exported ReloadConfig interface instead.
    dnephin committed Jun 8, 2021
    Configuration menu
    Copy the full SHA
    75f1e55 View commit details
    Browse the repository at this point in the history

Commits on Jun 9, 2021

  1. agent: fix a data race in a test

    The test was modifying a pointer to a struct that had been passed to
    another goroutine. Instead create a new struct to modify.
    
    ```
    WARNING: DATA RACE
    Write at 0x00c01407c3c0 by goroutine 832:
      github.com/hashicorp/consul/agent.TestServiceManager_PersistService_API()
          /home/daniel/pers/code/consul/agent/service_manager_test.go:446 +0x1d86
      testing.tRunner()
          /usr/lib/go/src/testing/testing.go:1193 +0x202
    
    Previous read at 0x00c01407c3c0 by goroutine 938:
      reflect.typedmemmove()
          /usr/lib/go/src/runtime/mbarrier.go:177 +0x0
      reflect.Value.Set()
          /usr/lib/go/src/reflect/value.go:1569 +0x13b
      github.com/mitchellh/copystructure.(*walker).Primitive()
          /home/daniel/go/pkg/mod/github.com/mitchellh/copystructure@v1.0.0/copystructure.go:289 +0x190
      github.com/mitchellh/reflectwalk.walkPrimitive()
          /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:252 +0x31b
      github.com/mitchellh/reflectwalk.walk()
          /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:179 +0x24d
      github.com/mitchellh/reflectwalk.walkStruct()
          /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:386 +0x4ec
      github.com/mitchellh/reflectwalk.walk()
          /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:188 +0x656
      github.com/mitchellh/reflectwalk.walkStruct()
          /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:386 +0x4ec
      github.com/mitchellh/reflectwalk.walk()
          /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:188 +0x656
      github.com/mitchellh/reflectwalk.Walk()
          /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:92 +0x164
      github.com/mitchellh/copystructure.Config.Copy()
          /home/daniel/go/pkg/mod/github.com/mitchellh/copystructure@v1.0.0/copystructure.go:69 +0xe7
      github.com/mitchellh/copystructure.Copy()
          /home/daniel/go/pkg/mod/github.com/mitchellh/copystructure@v1.0.0/copystructure.go:13 +0x84
      github.com/hashicorp/consul/agent.mergeServiceConfig()
          /home/daniel/pers/code/consul/agent/service_manager.go:362 +0x56
      github.com/hashicorp/consul/agent.(*serviceConfigWatch).handleUpdate()
          /home/daniel/pers/code/consul/agent/service_manager.go:279 +0x250
      github.com/hashicorp/consul/agent.(*serviceConfigWatch).runWatch()
          /home/daniel/pers/code/consul/agent/service_manager.go:246 +0x2d4
    
    Goroutine 832 (running) created at:
      testing.(*T).Run()
          /usr/lib/go/src/testing/testing.go:1238 +0x5d7
      testing.runTests.func1()
          /usr/lib/go/src/testing/testing.go:1511 +0xa6
      testing.tRunner()
          /usr/lib/go/src/testing/testing.go:1193 +0x202
      testing.runTests()
          /usr/lib/go/src/testing/testing.go:1509 +0x612
      testing.(*M).Run()
          /usr/lib/go/src/testing/testing.go:1417 +0x3b3
      main.main()
          _testmain.go:1181 +0x236
    
    Goroutine 938 (running) created at:
      github.com/hashicorp/consul/agent.(*serviceConfigWatch).start()
          /home/daniel/pers/code/consul/agent/service_manager.go:223 +0x4e4
      github.com/hashicorp/consul/agent.(*ServiceManager).AddService()
          /home/daniel/pers/code/consul/agent/service_manager.go:98 +0x344
      github.com/hashicorp/consul/agent.(*Agent).addServiceLocked()
          /home/daniel/pers/code/consul/agent/agent.go:1942 +0x2e4
      github.com/hashicorp/consul/agent.(*Agent).AddService()
          /home/daniel/pers/code/consul/agent/agent.go:1929 +0x337
      github.com/hashicorp/consul/agent.TestServiceManager_PersistService_API()
          /home/daniel/pers/code/consul/agent/service_manager_test.go:400 +0x17c4
      testing.tRunner()
          /usr/lib/go/src/testing/testing.go:1193 +0x202
    
    ```
    dnephin committed Jun 9, 2021
    Configuration menu
    Copy the full SHA
    d9e3f12 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    76c91ec View commit details
    Browse the repository at this point in the history
  3. ci: attach /go/bin to go-test-race job

    So that the tests in command can use the consul binary
    dnephin committed Jun 9, 2021
    Configuration menu
    Copy the full SHA
    81eb0e7 View commit details
    Browse the repository at this point in the history
  4. Update serf

    To pick up data race fixes
    dnephin committed Jun 9, 2021
    Configuration menu
    Copy the full SHA
    bfc2ebd View commit details
    Browse the repository at this point in the history

Commits on Jun 10, 2021

  1. Fix a data race in TestACLResolver_Client

    By setting the hash when we create the policy.
    
    ```
    WARNING: DATA RACE
    Read at 0x00c0028b4b10 by goroutine 1182:
      github.com/hashicorp/consul/agent/structs.(*ACLPolicy).SetHash()
          /home/daniel/pers/code/consul/agent/structs/acl.go:701 +0x40d
      github.com/hashicorp/consul/agent/structs.ACLPolicies.resolveWithCache()
          /home/daniel/pers/code/consul/agent/structs/acl.go:779 +0xfe
      github.com/hashicorp/consul/agent/structs.ACLPolicies.Compile()
          /home/daniel/pers/code/consul/agent/structs/acl.go:809 +0xf1
      github.com/hashicorp/consul/agent/consul.(*ACLResolver).ResolveTokenToIdentityAndAuthorizer()
          /home/daniel/pers/code/consul/agent/consul/acl.go:1226 +0x6ef
      github.com/hashicorp/consul/agent/consul.resolveTokenAsync()
          /home/daniel/pers/code/consul/agent/consul/acl_test.go:66 +0x5c
    
    Previous write at 0x00c0028b4b10 by goroutine 1509:
      github.com/hashicorp/consul/agent/structs.(*ACLPolicy).SetHash()
          /home/daniel/pers/code/consul/agent/structs/acl.go:730 +0x3a8
      github.com/hashicorp/consul/agent/structs.ACLPolicies.resolveWithCache()
          /home/daniel/pers/code/consul/agent/structs/acl.go:779 +0xfe
      github.com/hashicorp/consul/agent/structs.ACLPolicies.Compile()
          /home/daniel/pers/code/consul/agent/structs/acl.go:809 +0xf1
      github.com/hashicorp/consul/agent/consul.(*ACLResolver).ResolveTokenToIdentityAndAuthorizer()
          /home/daniel/pers/code/consul/agent/consul/acl.go:1226 +0x6ef
      github.com/hashicorp/consul/agent/consul.resolveTokenAsync()
          /home/daniel/pers/code/consul/agent/consul/acl_test.go:66 +0x5c
    
    Goroutine 1182 (running) created at:
      github.com/hashicorp/consul/agent/consul.TestACLResolver_Client.func4()
          /home/daniel/pers/code/consul/agent/consul/acl_test.go:1669 +0x459
      testing.tRunner()
          /usr/lib/go/src/testing/testing.go:1193 +0x202
    
    Goroutine 1509 (running) created at:
      github.com/hashicorp/consul/agent/consul.TestACLResolver_Client.func4()
          /home/daniel/pers/code/consul/agent/consul/acl_test.go:1668 +0x415
      testing.tRunner()
          /usr/lib/go/src/testing/testing.go:1193 +0x202
    ```
    dnephin committed Jun 10, 2021
    Configuration menu
    Copy the full SHA
    6165c15 View commit details
    Browse the repository at this point in the history
  2. Remove a racy test

    This test is super racy, and we already have test coverage for this
    functionality in the agent/cache package with TestCacheThrottle.
    
    Instead of spending time rewriting this test, let's remove it.
    
    ```
    WARNING: DATA RACE
    Read at 0x00c01de410fc by goroutine 735:
      github.com/hashicorp/consul/agent.TestCacheRateLimit.func1()
          /home/daniel/pers/code/consul/agent/agent_test.go:1024 +0x9af
      github.com/hashicorp/consul/testrpc.WaitForTestAgent()
          /home/daniel/pers/code/consul/testrpc/wait.go:99 +0x209
      github.com/hashicorp/consul/agent.TestCacheRateLimit.func1()
          /home/daniel/pers/code/consul/agent/agent_test.go:966 +0x1ad
      testing.tRunner()
          /usr/lib/go/src/testing/testing.go:1193 +0x202
    
    Previous write at 0x00c01de410fc by goroutine 605:
      github.com/hashicorp/consul/agent.TestCacheRateLimit.func1.2()
          /home/daniel/pers/code/consul/agent/agent_test.go:998 +0xe9
    
    Goroutine 735 (running) created at:
      testing.(*T).Run()
          /usr/lib/go/src/testing/testing.go:1238 +0x5d7
      github.com/hashicorp/consul/agent.TestCacheRateLimit()
          /home/daniel/pers/code/consul/agent/agent_test.go:961 +0x375
      testing.tRunner()
          /usr/lib/go/src/testing/testing.go:1193 +0x202
    
    Goroutine 605 (finished) created at:
      github.com/hashicorp/consul/agent.TestCacheRateLimit.func1()
          /home/daniel/pers/code/consul/agent/agent_test.go:1022 +0x91e
      github.com/hashicorp/consul/testrpc.WaitForTestAgent()
          /home/daniel/pers/code/consul/testrpc/wait.go:99 +0x209
      github.com/hashicorp/consul/agent.TestCacheRateLimit.func1()
          /home/daniel/pers/code/consul/agent/agent_test.go:966 +0x1ad
      testing.tRunner()
          /usr/lib/go/src/testing/testing.go:1193 +0x202
    ```
    dnephin committed Jun 10, 2021
    Configuration menu
    Copy the full SHA
    40876bc View commit details
    Browse the repository at this point in the history