Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix data race in Postgres engine on connection close #32650

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

Tener
Copy link
Contributor

@Tener Tener commented Sep 27, 2023

This fixes the data race noticed by a new test in another PR: #32485. The test has since changed enough not to trigger the data race anymore, but the original formulation still triggers it often enough.

WARNING: DATA RACE
Read at 0x00c005f72880 by goroutine 7359:
  github.com/jackc/pgconn.(*PgConn).IsClosed()
      /go/pkg/mod/github.com/jackc/pgconn@v1.14.1/pgconn.go:695 +0x49d
  github.com/gravitational/teleport/lib/srv/db/postgres.(*Engine).receiveFromServer.func1()
      /__w/teleport/teleport/lib/srv/db/postgres/engine.go:436 +0x4aa

Previous write at 0x00c005f72880 by goroutine 7325:
  github.com/jackc/pgconn.(*PgConn).Close()
      /go/pkg/mod/github.com/jackc/pgconn@v1.14.1/pgconn.go:627 +0x92
  github.com/gravitational/teleport/lib/srv/db/postgres.(*Engine).HandleConnection.func2()
      /__w/teleport/teleport/lib/srv/db/postgres/engine.go:167 +0x64
  runtime.deferreturn()
      /opt/go/src/runtime/panic.go:477 +0x30
  github.com/gravitational/teleport/lib/srv/db/common.(*reportingEngine).HandleConnection()
      /__w/teleport/teleport/lib/srv/db/common/reporting.go:87 +0x21a
  github.com/gravitational/teleport/lib/srv/db.(*Server).handleConnection()
      /__w/teleport/teleport/lib/srv/db/server.go:1021 +0x764
  github.com/gravitational/teleport/lib/srv/db.(*Server).HandleConnection()
      /__w/teleport/teleport/lib/srv/db/server.go:945 +0x95a
  github.com/gravitational/teleport/lib/srv/db.(*testContext).startHandlingConnections.func1()
      /__w/teleport/teleport/lib/srv/db/access_test.go:1488 +0x4f

Goroutine 7359 (running) created at:
  github.com/gravitational/teleport/lib/srv/db/postgres.(*Engine).receiveFromServer()
      /__w/teleport/teleport/lib/srv/db/postgres/engine.go:[421](https://github.com/gravitational/teleport/actions/runs/6313772578/job/17142603054?pr=32485#step:10:425) +0x4cf
  github.com/gravitational/teleport/lib/srv/db/postgres.(*Engine).HandleConnection.func6()
      /__w/teleport/teleport/lib/srv/db/postgres/engine.go:180 +0x5d

Goroutine 7325 (running) created at:
  github.com/gravitational/teleport/lib/srv/db.(*testContext).startHandlingConnections()
      /__w/teleport/teleport/lib/srv/db/access_test.go:1488 +0x84
  github.com/gravitational/teleport/lib/srv/db.TestDatabaseServerAutoDisconnect.func4()
      /__w/teleport/teleport/lib/srv/db/server_test.go:201 +0x33

@github-actions github-actions bot added database-access Database access related issues and PRs size/sm labels Sep 27, 2023
@Tener Tener changed the title Fix data race in Postgres engine Fix data race in Postgres engine on connection close Sep 27, 2023
Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

This is fine but I think it can be even simpler.

lib/srv/db/postgres/engine.go Outdated Show resolved Hide resolved
lib/srv/db/postgres/engine.go Outdated Show resolved Hide resolved
@Tener Tener added this pull request to the merge queue Sep 29, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 29, 2023
@Tener Tener added this pull request to the merge queue Sep 29, 2023
Merged via the queue into master with commit 818d9ed Sep 29, 2023
25 checks passed
@Tener Tener deleted the tener/postgres-conn-close-race-fix branch September 29, 2023 07:42
@public-teleport-github-review-bot

@Tener See the table below for backport results.

Branch Result
branch/v12 Failed
branch/v13 Failed
branch/v14 Create PR

@Tener
Copy link
Contributor Author

Tener commented Sep 29, 2023

No need for v13 or v12 backports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v14 database-access Database access related issues and PRs size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants