upper/db

Data race in template writer when running concurrent tests with go test -race

mscno opened this issue · 0 comments

mscno commented

When running several tests concurrently, the go race detection results in an error in the template writer code:

==================
WARNING: DATA RACE
Read at 0x0001067fb8e0 by goroutine 34:
  github.com/upper/db/v4/internal/sqladapter/exql.(*Template).getTemplate()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/template.go:135 +0x88
  github.com/upper/db/v4/internal/sqladapter/exql.(*Template).MustCompile()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/template.go:95 +0x8c
  github.com/upper/db/v4/internal/sqladapter/exql.(*Statement).Compile()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/statement.go:118 +0x204
  github.com/upper/db/v4/adapter/cockroachdb.(*database).CompileStatement()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/database.go:126 +0x4c
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).compileStatement()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:907 +0x250
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).StatementQuery()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:835 +0x2a4
  github.com/upper/db/v4/internal/sqlbuilder.(*selector).IteratorContext()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqlbuilder/select.go:480 +0x34c
  github.com/upper/db/v4/internal/sqlbuilder.(*selector).Iterator()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqlbuilder/select.go:470 +0x58
  github.com/upper/db/v4/adapter/cockroachdb.(*database).LookupName()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/database.go:161 +0x1c4
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).BindDB()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:544 +0x1c0
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).Open()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:373 +0x8c
  github.com/upper/db/v4/internal/sqladapter.(*sqlAdapterWrapper).OpenDSN()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/sqladapter.go:56 +0x60
  github.com/upper/db/v4/adapter/cockroachdb.Open()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/cockroachdb.go:40 +0x3f4

...

Previous write at 0x0001067fb8e0 by goroutine 35:
  github.com/upper/db/v4/internal/sqladapter/exql.(*Template).getTemplate()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/template.go:136 +0xa4
  github.com/upper/db/v4/internal/sqladapter/exql.(*Template).MustCompile()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/template.go:95 +0x8c
  github.com/upper/db/v4/internal/sqladapter/exql.(*Statement).Compile()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/exql/statement.go:118 +0x204
  github.com/upper/db/v4/adapter/cockroachdb.(*database).CompileStatement()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/database.go:126 +0x4c
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).compileStatement()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:907 +0x250
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).StatementQuery()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:835 +0x2a4
  github.com/upper/db/v4/internal/sqlbuilder.(*selector).IteratorContext()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqlbuilder/select.go:480 +0x34c
  github.com/upper/db/v4/internal/sqlbuilder.(*selector).Iterator()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqlbuilder/select.go:470 +0x58
  github.com/upper/db/v4/adapter/cockroachdb.(*database).LookupName()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/database.go:161 +0x1c4
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).BindDB()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:544 +0x1c0
  github.com/upper/db/v4/internal/sqladapter.(*sessionWithContext).Open()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/session.go:373 +0x8c
  github.com/upper/db/v4/internal/sqladapter.(*sqlAdapterWrapper).OpenDSN()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/internal/sqladapter/sqladapter.go:56 +0x60
  github.com/upper/db/v4/adapter/cockroachdb.Open()
      /Users/user/go/pkg/mod/github.com/upper/db/v4@v4.7.0/adapter/cockroachdb/cockroachdb.go:40 +0x3f4


func (t *Template) getTemplate(k string) (*template.Template, bool) {
	t.templateMutex.RLock()
	defer t.templateMutex.RUnlock()

	if t.templateMap == nil { // Read done here in other go routine at the same time
		t.templateMap = make(map[string]*template.Template) // WRITE DONE TO t.templateMap here without read lock
	}

	v, ok := t.templateMap[k]
	return v, ok
}

func (t *Template) setTemplate(k string, v *template.Template) {
	t.templateMutex.Lock()
	defer t.templateMutex.Unlock()

	t.templateMap[k] = v
}

Can this be fixed by a simple PR wrapping the assignment of the empty map in a Write Lock (mux.Lock) ?

E.g. like so:

func (t *Template) getTemplate(k string) (*template.Template, bool) {
	t.templateMutex.RLock()
	defer t.templateMutex.RUnlock()

	if t.templateMap == nil { 
               t.templateMutex.Lock() // Lock prevents other routines from achieving a reader. Maybe add another check to see if it is still nil after getting the Write Lock?
   	       defer t.templateMutex.Unlock()
		t.templateMap = make(map[string]*template.Template)
	}

	v, ok := t.templateMap[k]
	return v, ok
}

func (t *Template) setTemplate(k string, v *template.Template) {
	t.templateMutex.Lock()
	defer t.templateMutex.Unlock()

	t.templateMap[k] = v
}