chore(spanner): fix some go vet issues by egonelbre · Pull Request #9758 · googleapis/google-cloud-go

egonelbre · 2024-04-12T09:38:51Z

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

These are not problems per se, however they end up as noise in `go vet` output.

…able structs While the copying in these locations doesn't change the result, the copies end up as noise in go vet output.

noahdietz

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

gcf-merge-on-green · 2024-05-02T04:48:15Z

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 · 2024-05-02T06:35:02Z

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 · 2024-05-02T15:37:50Z

Running presubmits again, hopefully just a flake

noahdietz · 2024-05-02T15:42:55Z

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 · 2024-05-02T16:33:16Z

=== 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 · 2024-05-02T18:48:31Z

@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 · 2024-05-03T09:57:22Z

@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.

egonelbre added 3 commits April 12, 2024 12:16

chore(spanner): add missing field keys

9e6d05a

These are not problems per se, however they end up as noise in `go vet` output.

chore(spanner): fix go vet complaints in tests about copying non-copy…

fe09ce5

…able structs While the copying in these locations doesn't change the result, the copies end up as noise in go vet output.

chore(spanner): call cancel on context timeout

182aef9

egonelbre requested review from a team April 12, 2024 09:38

product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Apr 12, 2024

rahul2393 approved these changes Apr 16, 2024

View reviewed changes

Merge branch 'main' into govet

46c5d48

egonelbre mentioned this pull request Apr 16, 2024

vet.sh: linting does not work for pull requests #9784

Open

egonelbre and others added 2 commits April 30, 2024 10:44

Merge branch 'main' into govet

a9a8e7d

Merge branch 'main' into govet

b654dc1

noahdietz added the automerge: exact Summon MOG for automerging, but approvals need to be against the latest commit label May 1, 2024

noahdietz approved these changes May 1, 2024

View reviewed changes

gcf-merge-on-green Bot removed the automerge: exact Summon MOG for automerging, but approvals need to be against the latest commit label May 2, 2024

rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 2, 2024