Skip to content

Fix data race in Postgres engine on connection close#32650

Merged
Tener merged 2 commits intomasterfrom
tener/postgres-conn-close-race-fix
Sep 29, 2023
Merged

Fix data race in Postgres engine on connection close#32650
Tener merged 2 commits intomasterfrom
tener/postgres-conn-close-race-fix

Conversation

@Tener
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread lib/srv/db/postgres/engine.go Outdated
Comment thread lib/srv/db/postgres/engine.go Outdated
@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
@Tener Tener deleted the tener/postgres-conn-close-race-fix branch September 29, 2023 07:42
@public-teleport-github-review-bot
Copy link
Copy Markdown

@Tener See the table below for backport results.

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

@Tener
Copy link
Copy Markdown
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

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