chore(spanner): fix some go vet issues

[egonelbre]

This fixes some trivial go vet issues. Each commit is dedicated to specific issue class (mostly).

[noahdietz]

Approving again since this went a tad stale. Thanks for the tweaks @egonelbre

[gcf-merge-on-green]

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

[egonelbre]

Looks like this failed:

=== NAME  TestMaintainer
    session_test.go:1570: after 15s waiting, got numOpened sessions mismatch
        Got: 23
        Want: 30
--- FAIL: TestMaintainer (15.06s)

But, as far as I can tell this PR doesn't change anything about it.

[noahdietz]

Running presubmits again, hopefully just a flake

[noahdietz]

Running presubmits again, hopefully just a flake

Looks like it cleared. I'll baby sit the PR to submission - I'm merging other stuff atm

[noahdietz]

=== RUN   TestClient_DoForEachRow_ShouldNotEndSpanWithIteratorDoneError
==================
WARNING: DATA RACE
Write at 0x00c000e00048 by goroutine 37939:
  cloud.google.com/go/internal/testutil.(*TestExporter).ExportSpan()
      /go/pkg/mod/cloud.google.com/go@v0.112.2/internal/testutil/trace.go:59 +0x164
  go.opencensus.io/trace.(*span).End.func1()
      /go/pkg/mod/go.opencensus.io@v0.24.0/trace/trace.go:298 +0x24d
  sync.(*Once).doSlow()
      /usr/local/go/src/sync/once.go:74 +0x101
  sync.(*Once).Do()
      /usr/local/go/src/sync/once.go:65 +0x46
  go.opencensus.io/trace.(*span).End()
      /go/pkg/mod/go.opencensus.io@v0.24.0/trace/trace.go:287 +0xa4
  go.opencensus.io/trace.(*Span).End()
      /go/pkg/mod/go.opencensus.io@v0.24.0/trace/trace_api.go:171 +0x15b
  cloud.google.com/go/internal/trace.EndSpan()
      /go/pkg/mod/cloud.google.com/go@v0.112.2/internal/trace/trace.go:125 +0x13c
  cloud.google.com/go/spanner.(*sessionClient).executeBatchCreateSessions.func1()
      /tmpfs/src/google-cloud-go/spanner/sessionclient.go:247 +0x44
  runtime.deferreturn()
      /usr/local/go/src/runtime/panic.go:476 +0x32
  cloud.google.com/go/spanner.(*sessionClient).batchCreateSessions.func2()
      /tmpfs/src/google-cloud-go/spanner/sessionclient.go:234 +0x96

Previous read at 0x00c000e00048 by goroutine 38201:
  cloud.google.com/go/spanner.TestClient_DoForEachRow_ShouldNotEndSpanWithIteratorDoneError()
      /tmpfs/src/google-cloud-go/spanner/client_test.go:4554 +0x30b
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x47

Goroutine 37939 (running) created at:
  cloud.google.com/go/spanner.(*sessionClient).batchCreateSessions()
      /tmpfs/src/google-cloud-go/spanner/sessionclient.go:234 +0x51d
  cloud.google.com/go/spanner.(*sessionPool).growPoolLocked()
      /tmpfs/src/google-cloud-go/spanner/session.go:812 +0x1ae
  cloud.google.com/go/spanner.(*sessionPool).initPool()
      /tmpfs/src/google-cloud-go/spanner/session.go:802 +0xaa
  cloud.google.com/go/spanner.newSessionPool()
      /tmpfs/src/google-cloud-go/spanner/session.go:692 +0xb6e
  cloud.google.com/go/spanner.newClientWithConfig()
      /tmpfs/src/google-cloud-go/spanner/client.go:464 +0xc84
  cloud.google.com/go/spanner.NewClientWithConfig()
      /tmpfs/src/google-cloud-go/spanner/client.go:372 +0x137
  cloud.google.com/go/spanner.NewClient()
      /tmpfs/src/google-cloud-go/spanner/client.go:366 +0x6e
  cloud.google.com/go/spanner.makeClient()
      /tmpfs/src/google-cloud-go/spanner/client_test.go:133 +0x2c6
  cloud.google.com/go/spanner.TestClient_EmulatorWithCredentialsFile()
      /tmpfs/src/google-cloud-go/spanner/client_test.go:4285 +0x184
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x47

Goroutine 38201 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1493 +0x75d
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1846 +0x99
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x216
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1844 +0x7ec
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1726 +0xa84
  cloud.google.com/go/spanner.TestMain()
      /tmpfs/src/google-cloud-go/spanner/integration_test.go:398 +0x10a
  main.main()
      _testmain.go:841 +0x324
==================
    testing.go:1319: race detected during execution of test
--- FAIL: TestClient_DoForEachRow_ShouldNotEndSpanWithIteratorDoneError (0.03s)

Failure on last run was a data race...we should probably look into that separately @rahul2393 @harshachinta

[egonelbre]

@noahdietz I usually can reproduce similar issues with targeting specific tests and then running the tests multiple times... e.g. go test -run "TestClient_DoForEachRow_ShouldNotEndSpanWithIteratorDoneError| TestClient_EmulatorWithCredentialsFile" -race -count 10.

As far as I can tell from the trace -- it's happening because the clients are not waiting for every started goroutine to be finished. Nothing is waiting this goroutine to be finished:

go sc.executeBatchCreateSessions(client, createCountForChannel, sc.sessionLabels, sc.md, consumer)

And it seems the tracing is using global vars, which then ends up causing data races.

Without knowing the exact details, I would add a waitgroup / errgoup into sessionClient and when calling close it then must wait for all the started goroutines to be finished.

[egonelbre]

@noahdietz I ended up creating #10095 -- I'm not yet sure whether it'll pass the test, but at least it should reduce the likelihood of a race.