Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

gitserver: disk-info: make test hit two separate gitserver addresses#57318

Merged
ggilmore merged 1 commit into
mainfrom
hmm
Oct 4, 2023
Merged

gitserver: disk-info: make test hit two separate gitserver addresses#57318
ggilmore merged 1 commit into
mainfrom
hmm

Conversation

@ggilmore

@ggilmore ggilmore commented Oct 3, 2023

Copy link
Copy Markdown
Contributor

Depends on #57316

When I run go test -race on this, I get the following (expected) race condition that @eseliger 's #57316 PR will fix.

=== RUN   TestClient_SystemsInfo/GRPC
==================
WARNING: DATA RACE
Write at 0x00c000e40e9f by goroutine 112:
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.TestClient_SystemsInfo.func3.2.1.1()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1585 +0xe4
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.(*mockClient).DiskInfo()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1776 +0x74
  github.com/sourcegraph/sourcegraph/internal/gitserver.(*clientImplementor).getDiskInfo()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client.go:519 +0xe0
  github.com/sourcegraph/sourcegraph/internal/gitserver.(*clientImplementor).SystemsInfo.func1()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client.go:480 +0x74
  github.com/sourcegraph/conc/panics.(*Catcher).Try()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/panics/panics.go:23 +0x60
  github.com/sourcegraph/conc.(*WaitGroup).Go.func1()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/waitgroup.go:32 +0x74

Previous write at 0x00c000e40e9f by goroutine 111:
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.TestClient_SystemsInfo.func3.2.1.1()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1585 +0xe4
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.(*mockClient).DiskInfo()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1776 +0x74
  github.com/sourcegraph/sourcegraph/internal/gitserver.(*clientImplementor).getDiskInfo()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client.go:519 +0xe0
  github.com/sourcegraph/sourcegraph/internal/gitserver.(*clientImplementor).SystemsInfo.func1()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client.go:480 +0x74
  github.com/sourcegraph/conc/panics.(*Catcher).Try()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/panics/panics.go:23 +0x60
  github.com/sourcegraph/conc.(*WaitGroup).Go.func1()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/waitgroup.go:32 +0x74

Goroutine 112 (running) created at:
  github.com/sourcegraph/conc.(*WaitGroup).Go()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/waitgroup.go:30 +0xd4
  github.com/sourcegraph/sourcegraph/internal/gitserver.(*clientImplementor).SystemsInfo()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client.go:479 +0x174
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.TestClient_SystemsInfo.func2()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1539 +0x78
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.TestClient_SystemsInfo.func3()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1594 +0x30c
  testing.tRunner()
      /Users/ggilmore/go/go1.20.5/src/testing/testing.go:1576 +0x188
  testing.(*T).Run.func1()
      /Users/ggilmore/go/go1.20.5/src/testing/testing.go:1629 +0x40

Goroutine 111 (finished) created at:
  github.com/sourcegraph/conc.(*WaitGroup).Go()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/waitgroup.go:30 +0xd4
  github.com/sourcegraph/sourcegraph/internal/gitserver.(*clientImplementor).SystemsInfo()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client.go:479 +0x174
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.TestClient_SystemsInfo.func2()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1539 +0x78
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.TestClient_SystemsInfo.func3()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1594 +0x30c
  testing.tRunner()
      /Users/ggilmore/go/go1.20.5/src/testing/testing.go:1576 +0x188
  testing.(*T).Run.func1()
      /Users/ggilmore/go/go1.20.5/src/testing/testing.go:1629 +0x40
==================
==================
WARNING: DATA RACE
Read at 0x00c00044a420 by goroutine 112:
  github.com/sourcegraph/sourcegraph/internal/gitserver.(*clientImplementor).SystemsInfo.func1()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client.go:485 +0x1fc
  github.com/sourcegraph/conc/panics.(*Catcher).Try()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/panics/panics.go:23 +0x60
  github.com/sourcegraph/conc.(*WaitGroup).Go.func1()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/waitgroup.go:32 +0x74

Previous write at 0x00c00044a420 by goroutine 111:
  github.com/sourcegraph/sourcegraph/internal/gitserver.(*clientImplementor).SystemsInfo.func1()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client.go:485 +0x2b0
  github.com/sourcegraph/conc/panics.(*Catcher).Try()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/panics/panics.go:23 +0x60
  github.com/sourcegraph/conc.(*WaitGroup).Go.func1()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/waitgroup.go:32 +0x74

Goroutine 112 (running) created at:
  github.com/sourcegraph/conc.(*WaitGroup).Go()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/waitgroup.go:30 +0xd4
  github.com/sourcegraph/sourcegraph/internal/gitserver.(*clientImplementor).SystemsInfo()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client.go:479 +0x174
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.TestClient_SystemsInfo.func2()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1539 +0x78
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.TestClient_SystemsInfo.func3()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1594 +0x30c
  testing.tRunner()
      /Users/ggilmore/go/go1.20.5/src/testing/testing.go:1576 +0x188
  testing.(*T).Run.func1()
      /Users/ggilmore/go/go1.20.5/src/testing/testing.go:1629 +0x40

Goroutine 111 (finished) created at:
  github.com/sourcegraph/conc.(*WaitGroup).Go()
      /Users/ggilmore/go/pkg/mod/github.com/sourcegraph/conc@v0.2.0/waitgroup.go:30 +0xd4
  github.com/sourcegraph/sourcegraph/internal/gitserver.(*clientImplementor).SystemsInfo()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client.go:479 +0x174
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.TestClient_SystemsInfo.func2()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1539 +0x78
  github.com/sourcegraph/sourcegraph/internal/gitserver_test.TestClient_SystemsInfo.func3()
      /Users/ggilmore/dev/sourcegraph/internal/gitserver/client_test.go:1594 +0x30c
  testing.tRunner()
      /Users/ggilmore/go/go1.20.5/src/testing/testing.go:1576 +0x188
  testing.(*T).Run.func1()
      /Users/ggilmore/go/go1.20.5/src/testing/testing.go:1629 +0x40
==================
    testing.go:1446: race detected during execution of test
    --- FAIL: TestClient_SystemsInfo/GRPC (0.00s)


Test plan

This is adding unit tests

@cla-bot cla-bot Bot added the cla-signed label Oct 3, 2023
@sourcegraph-bot

sourcegraph-bot commented Oct 3, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 3a46572...989e5e8.

Notify File(s)
@eseliger internal/gitserver/BUILD.bazel
internal/gitserver/client.go
internal/gitserver/client_test.go

Comment thread internal/gitserver/client.go Outdated

@ggilmore ggilmore Oct 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BolajiOlajide I also fixed this. You're correctly using protojson.Marshal on the server side to encode the response, but you also need to use protojson.Unmarshal on the client side. The normal json decoder doesn't correctly parse the response otherwise.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good find!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ggilmore

@ggilmore

ggilmore commented Oct 3, 2023

Copy link
Copy Markdown
Contributor Author

@eseliger I don't know whether or not the race detector is enabled in CI, but it never would have caught this case in the first place since we were only using one goroutine to begin with.

@ggilmore ggilmore requested a review from a team October 3, 2023 18:58
@ggilmore ggilmore changed the base branch from main to es/conc-sysinfo October 3, 2023 19:20
@ggilmore ggilmore changed the base branch from es/conc-sysinfo to main October 3, 2023 19:20
Comment thread internal/gitserver/client.go Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good find!

@eseliger

eseliger commented Oct 3, 2023

Copy link
Copy Markdown
Member

@eseliger I don't know whether or not the race detector is enabled in CI, but it never would have caught this case in the first place since we were only using one goroutine to begin with.

Interesting, yeah 🤔
Generally, testing is a big topic we can spend more time on in our code.

@eseliger

eseliger commented Oct 3, 2023

Copy link
Copy Markdown
Member

the other PR has been merged!

Comment thread internal/gitserver/client.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ggilmore

@ggilmore ggilmore enabled auto-merge (squash) October 4, 2023 16:44
@ggilmore ggilmore merged commit 8a30d3c into main Oct 4, 2023
@ggilmore ggilmore deleted the hmm branch October 4, 2023 16:56
@sourcegraph-release-bot

Copy link
Copy Markdown
Collaborator

The backport to 5.2 failed at https://github.com/sourcegraph/sourcegraph/actions/runs/6409107875:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-5.2 5.2
# Navigate to the new working tree
cd .worktrees/backport-5.2
# Create a new branch
git switch --create backport-57318-to-5.2
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 8a30d3ca7b784367b7b2de0d38a51847ae83a5d9
# Push it to GitHub
git push --set-upstream origin backport-57318-to-5.2
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.2

If you encouter conflict, first resolve the conflict and stage all files, then run the commands below:

git cherry-pick --continue
# Push it to GitHub
git push --set-upstream origin backport-57318-to-5.2
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.2
  • Follow above instructions to backport the commit.
  • Create a pull request where the base branch is 5.2 and the compare/head branch is backport-57318-to-5.2., click here to create the pull request.
  • Make sure to tag @sourcegraph/release-guild in the pull request description.
  • Once the backport pull request is created, kindly remove the release-blocker from this pull request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport 5.2 backports cla-signed failed-backport-to-5.2 release-blocker Prevents us from releasing: https://about.sourcegraph.com/handbook/engineering/releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants