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

[prziborowski]

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 3db76c0

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]

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.

[dougm]

Thanks for the test case and details. Ideally clients would only have 1 thread attempting Login at a time. But we can also just put a sync.Mutex around SessionManager.userSession.