Skip to content

datastore: Fix race when connecting Postgres listener

Patrick Steinhardt requested to merge pks-psql-listener-race into master

The following race was hit in CI:

WARNING: DATA RACE
Write at 0x00c0005cbd50 by goroutine 157:
  gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore.(*PostgresListener).connect()
      /builds/gitlab-org/gitaly/internal/praefect/datastore/listener_postgres.go:125 +0x2a4
  gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore.NewPostgresListener()
      /builds/gitlab-org/gitaly/internal/praefect/datastore/listener_postgres.go:78 +0x4ab
  gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore.TestPostgresListener_Listen.func13()
      /builds/gitlab-org/gitaly/internal/praefect/datastore/listener_postgres_test.go:326 +0x3e4
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1193 +0x202

Previous read at 0x00c0005cbd50 by goroutine 87:
  gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore.(*PostgresListener).handleNotifications()
      /builds/gitlab-org/gitaly/internal/praefect/datastore/listener_postgres.go:179 +0x7c
  gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore.(*PostgresListener).handleNotifications-fm()
      /builds/gitlab-org/gitaly/internal/praefect/datastore/listener_postgres.go:174 +0x4a
  gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore.(*PostgresListener).async.func1()
      /builds/gitlab-org/gitaly/internal/praefect/datastore/listener_postgres.go:216 +0x72

Goroutine 157 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1238 +0x5d7
  gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore.TestPostgresListener_Listen()
      /builds/gitlab-org/gitaly/internal/praefect/datastore/listener_postgres_test.go:312 +0xaac
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1193 +0x202

Goroutine 87 (running) created at:
  gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore.(*PostgresListener).async()
      /builds/gitlab-org/gitaly/internal/praefect/datastore/listener_postgres.go:214 +0x73
  gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore.(*PostgresListener).connect.func1()
      /builds/gitlab-org/gitaly/internal/praefect/datastore/listener_postgres.go:111 +0x364
  github.com/lib/pq.(*Listener).emitEvent()
      /root/go/pkg/mod/github.com/lib/pq@v1.10.1/notify.go:797 +0x575
  github.com/lib/pq.(*Listener).listenerConnLoop()
      /root/go/pkg/mod/github.com/lib/pq@v1.10.1/notify.go:827 +0x519
  github.com/lib/pq.(*Listener).listenerMain()
      /root/go/pkg/mod/github.com/lib/pq@v1.10.1/notify.go:856 +0x38
==================
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x11923d1]

What happens is that we have a race when creating the new Postgres listener: pq.NewListener() will call the provided event callback whenever the state changes, and our event handler accesses the listener field. Given that the listener field is being assigned to based on the result of pq.NewListener(), we thus have a race between connecting to the database and assigning to the field.

Fix this race by synchronizing on the assignment of the field.s

Merge request reports