go-pg / pg

Golang ORM with focus on PostgreSQL features and performance
https://pg.uptrace.dev/
BSD 2-Clause "Simplified" License
5.67k stars 404 forks source link

fix(notify): Fixed race condition in listener. #1963

Closed elliotcourant closed 2 years ago

elliotcourant commented 2 years ago

This resolves a race condition in listener where ln.closed was being read without a mutex at the same time it could be written. Just move the mutex unlock for Unlisten down, but not defer it so that the mutex still holds the lock properly. But does not cause a deadlock with release conn.

==================
    testing.go:1319: race detected during execution of test
WARNING: DATA RACE
Write at 0x00c000614098 by goroutine 38:
  github.com/go-pg/pg/v10.(*Listener).Close()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:146 +0x128
  github.com/go-pg/pg/v10.(*Listener).connWithLock()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:69 +0x219
  github.com/go-pg/pg/v10.(*Listener).ReceiveTimeout()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:226 +0x7b
  github.com/go-pg/pg/v10.(*Listener).Receive()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:218 +0xd3
  github.com/go-pg/pg/v10.(*Listener).initChannel.func1()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:285 +0x3d2

Previous read at 0x00c000614098 by goroutine 36:
  github.com/go-pg/pg/v10.(*Listener).conn()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:78 +0x64
  github.com/go-pg/pg/v10.(*Listener).Unlisten()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:190 +0x166

Goroutine 38 (running) created at:
  github.com/go-pg/pg/v10.(*Listener).initChannel()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:279 +0x23a
  github.com/go-pg/pg/v10.(*Listener).channel.func1()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:260 +0x3e
  sync.(*Once).doSlow()
      /usr/local/opt/go/libexec/src/sync/once.go:74 +0x101
  sync.(*Once).Do()
      /usr/local/opt/go/libexec/src/sync/once.go:65 +0x46
  github.com/go-pg/pg/v10.(*Listener).channel()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:259 +0x6d
  github.com/go-pg/pg/v10.(*Listener).Channel()
      /Users/elliotcourant/go/pkg/mod/github.com/go-pg/pg/v10@v10.10.6/listener.go:249 +0x71

Goroutine 36 (running) created at:
  github.com/monetr/monetr/pkg/pubsub.(*postgresPubSub).Subscribe()
      /Users/elliotcourant/monetr/monetr/pkg/pubsub/pubsub.go:84 +0x664
vmihailenco commented 2 years ago

Looks good. You can also do something like this:

    ln.mu.Lock()
    ln.channels = removeIfExists(ln.channels, channels...)
    cn, err := ln.conn(ctx)
    ln.mu.Unlock()
elliotcourant commented 2 years ago

I think this is a cleaner change, I’ll make that adjustment today On Oct 4, 2022 at 6:24 AM -0500, Vladimir Mihailenco @.***>, wrote:

Looks good. You can also do something like this: ln.mu.Lock() ln.channels = removeIfExists(ln.channels, channels...) cn, err := ln.conn(ctx) ln.mu.Unlock() — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>