vmware / govmomi

Go library for the VMware vSphere API
Apache License 2.0
2.3k stars 910 forks source link

[BUG] Data race when trying to use keep alive that re-login #3571

Open prziborowski opened 16 hours ago

prziborowski commented 16 hours ago

Describe the bug

I am seeing that if I try to perform a Login, that it will modify the userSession object. And if I'm also calling something like UserSession(), or maybe SessionIsActive, it is reading the object. I'm not sure if I should be adding locks on the client code or not. https://github.com/vmware-tanzu/vm-operator/blob/main/pkg/providers/vsphere/client/client_test.go#L305,L356 is a somewhat contrived case of the race.

To Reproduce

I wrote a small test-case that just loops forever (obviously not a candidate for checking in if it no longer hits the data race):

func TestIsSessionActive(t *testing.T) {

   simulator.Test(func(ctx context.Context, c *vim25.Client) {
      m := session.NewManager(c)
      m2 := session.NewManager(c)

      err := m.Logout(ctx)
      if err != nil {
         t.Fatal(err)
      }

      c.RoundTripper = vim25.Retry(c.Client, vim25.RetryTemporaryNetworkError, 3)
      c.RoundTripper = session.KeepAliveHandler(c.RoundTripper, time.Millisecond, func(soap.RoundTripper) error {
         active, _ := m.SessionIsActive(ctx)
         if !active {
            err := m.Login(ctx, simulator.DefaultLogin)
            if err != nil {
               t.Fatal(err)
            }
         }
         return nil
      })

      err = m.Login(ctx, simulator.DefaultLogin) // starts the keep alive routine
      if err != nil {
         t.Fatal(err)
      }

      for {
         sess, err := m.UserSession(ctx)
         if err != nil {
            t.Fatal(err)
         }
         err = m2.TerminateSession(ctx, []string{sess.Key})
         if err != nil {
            t.Fatal(err)
         }
      }
   })
}

Expected behavior Either hoping that it doesn't race here, or possibly suggestion on what locking structure to add from client.

Affected version commit 3db76c0b19c97fc81c9e4339c054cb4c31cf5b93

Screenshots/Debug Output

$ go test -v -race -run TestIsSessionActive github.com/vmware/govmomi/session
=== RUN   TestIsSessionActive
==================
WARNING: DATA RACE
Read at 0x00c0000c6f88 by goroutine 53:
  github.com/vmware/govmomi/session.(*Manager).SessionIsActive()
      /Users/nathanp/govmomi/session/manager.go:208 +0x5c
  github.com/vmware/govmomi/session_test.TestIsSessionActive.func1.1()
      /Users/nathanp/govmomi/session/keep_alive_test.go:78 +0x58
  github.com/vmware/govmomi/session_test.TestIsSessionActive.func1.KeepAliveHandler.2()
      /Users/nathanp/govmomi/session/keep_alive.go:36 +0x46
  github.com/vmware/govmomi/session/keepalive.(*handler).Start.func1()
      /Users/nathanp/govmomi/session/keepalive/handler.go:124 +0x132

Previous write at 0x00c0000c6f88 by goroutine 7:
  github.com/vmware/govmomi/session.(*Manager).Login()
      /Users/nathanp/govmomi/session/manager.go:105 +0x350
  github.com/vmware/govmomi/session_test.TestIsSessionActive.func1()
      /Users/nathanp/govmomi/session/keep_alive_test.go:88 +0x819
  github.com/vmware/govmomi/session_test.TestIsSessionActive.Test.func2()
      /Users/nathanp/govmomi/simulator/model.go:926 +0x4b
  github.com/vmware/govmomi/simulator.(*Model).Run()
      /Users/nathanp/govmomi/simulator/model.go:904 +0x345
  github.com/vmware/govmomi/simulator.Run()
      /Users/nathanp/govmomi/simulator/model.go:916 +0x38d
  github.com/vmware/govmomi/simulator.Test()
      /Users/nathanp/govmomi/simulator/model.go:925 +0x95
  github.com/vmware/govmomi/session_test.TestIsSessionActive()
      /Users/nathanp/govmomi/session/keep_alive_test.go:67 +0x25
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1689 +0x21e
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1742 +0x44

Goroutine 53 (running) created at:
  github.com/vmware/govmomi/session/keepalive.(*handler).Start()
      /Users/nathanp/govmomi/session/keepalive/handler.go:116 +0x1d3
  github.com/vmware/govmomi/session/keepalive.(*HandlerSOAP).RoundTrip()
      /Users/nathanp/govmomi/session/keepalive/handler.go:171 +0x13b
  github.com/vmware/govmomi/vim25.(*Client).RoundTrip()
      /Users/nathanp/govmomi/vim25/client.go:89 +0x9a
  github.com/vmware/govmomi/vim25/methods.Login()
      /Users/nathanp/govmomi/vim25/methods/methods.go:8259 +0x161
  github.com/vmware/govmomi/session.(*Manager).Login()
      /Users/nathanp/govmomi/session/manager.go:100 +0x32d
  github.com/vmware/govmomi/session_test.TestIsSessionActive.func1()
      /Users/nathanp/govmomi/session/keep_alive_test.go:88 +0x819
  github.com/vmware/govmomi/session_test.TestIsSessionActive.Test.func2()
      /Users/nathanp/govmomi/simulator/model.go:926 +0x4b
  github.com/vmware/govmomi/simulator.(*Model).Run()
      /Users/nathanp/govmomi/simulator/model.go:904 +0x345
  github.com/vmware/govmomi/simulator.Run()
      /Users/nathanp/govmomi/simulator/model.go:916 +0x38d
  github.com/vmware/govmomi/simulator.Test()
      /Users/nathanp/govmomi/simulator/model.go:925 +0x95
  github.com/vmware/govmomi/session_test.TestIsSessionActive()
      /Users/nathanp/govmomi/session/keep_alive_test.go:67 +0x25
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1689 +0x21e
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1742 +0x44

Additional context Add any other context about the problem here.

github-actions[bot] commented 16 hours ago

Howdy 🖐   prziborowski ! Thank you for your interest in this project. We value your feedback and will respond soon.

If you want to contribute to this project, please make yourself familiar with the CONTRIBUTION guidelines.